Re: Segfault in jit tuple deforming on arm64 due to LLVM issue

From: Anthonin Bonnefoy <anthonin(dot)bonnefoy(at)datadoghq(dot)com>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: Daniel Gustafsson <daniel(at)yesql(dot)se>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Segfault in jit tuple deforming on arm64 due to LLVM issue
Date: 2024-11-05 10:31:48
Message-ID: CAO6_XqpAQ7bsB0yHF5JtRn-=diRY8-t8Xcdng8rxTH2UfuAVxw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Nov 5, 2024 at 9:00 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> Reasoning: Old LLVM required C++11. LLVM 9 switched to C++14. LLVM
> 14 switched C++17. Pretty soon they'll flip to C++20 or C++23, they
> don't mess around. The corresponding -std=c++XX flag finishes up in
> our compile lines, because llvm-config --cxxflags spits it out, to
> match the features they're using in headers that we include (easy to
> spot examples being std::make_unique (C++14) and std::string_view
> (C++17)), so you might say that PostgreSQL indirectly chases C++
> standards much faster than it chases C standards. This particular
> code is a special case because it's guarded for LLVM 12+ only, so it's
> OK to use C++14 in that limited context even in back branches. We
> have to be careful that it doesn't contain C++17 code since it came
> from recent LLVM, but it doesn't seem to by inspection, and you can
> check on a machine with CXX=g++ and LLVM 14 on Linux, which uses
> -std=c++14 and fails if you add a use of <string_view> and
> std::string_view. (Warning: the system C++ standard library on Macs
> and other Clang-based systems doesn't have enough version guards so it
> won't complain, but GCC and its standard library will explicitly tell
> you not to use C++17 features in a C++14 program.)

I think the switch to C++14 happened with LLVM 10[0] while the C++17
switch happened with LLVM 16[1]. Double checking on an ubuntu jammy
(can't install earlier llvm version than 12 on those):

VERSIONS=(20 19 18 17 16 15 14 13 12)
for version in ${VERSIONS[(at)]}; do
llvm-config-$version --cxxflags
done

-I/usr/lib/llvm-20/include -std=c++17 -fno-exceptions
-funwind-tables -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-19/include -std=c++17 -fno-exceptions
-funwind-tables -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-18/include -std=c++17 -fno-exceptions
-funwind-tables -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-17/include -std=c++17 -fno-exceptions
-funwind-tables -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-16/include -std=c++17 -fno-exceptions -D_GNU_SOURCE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-15/include -std=c++14 -fno-exceptions -D_GNU_SOURCE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-14/include -std=c++14 -fno-exceptions -D_GNU_SOURCE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-13/include -std=c++14 -fno-exceptions -D_GNU_SOURCE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-I/usr/lib/llvm-12/include -std=c++14 -fno-exceptions -D_GNU_SOURCE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS

Which is still fine since, as you said, the code only applied for
LLVM12+ which will always have at least c++14. I've tried compiling
and running against all those llvm versions and the associated stdc++
version earlier in the thread[2] and had no issues.

> If there are no further comments, I'm going to push this to all
> branches tomorrow morning. For master only, I will remove the #if
> condition and comment about LLVM 12+, as we now require 14+.

Patch looks good to me.

[0]: https://github.com/llvm/llvm-project/blob/release/10.x/llvm/CMakeLists.txt#L53
[1]: https://github.com/llvm/llvm-project/blob/release/16.x/llvm/CMakeLists.txt#L70
[2]: https://www.postgresql.org/message-id/CAO6_XqqxEQ%3DJY%2BtYO-KQn3_pKQ3O-mPojcwG54L5eptiu1cSJQ%40mail.gmail.com

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2024-11-05 10:55:35 Re: Pgoutput not capturing the generated columns
Previous Message Nishant Sharma 2024-11-05 10:30:30 Re: on_error table, saving error info to a table