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

From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Anthonin Bonnefoy <anthonin(dot)bonnefoy(at)datadoghq(dot)com>, 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 07:59:33
Message-ID: CA+hUKGL2SFM+3MmuBp5mE3PdU2eabqKDfrK_ZYJGtfaMO0_p2g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Oct 31, 2024 at 9:49 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> > On 31 Oct 2024, at 06:48, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > I guess at a minimum a copy of the licence would need to appear somewhere
>
> That's my interpretation of it as well.
>
> > perhaps under src/backend/jit/llvm?
>
> Since SectionMemoryManager.h is in src/backend/jit I wonder if it should be a
> placed in a section in src/backend/jit/README with an overview of the what and
> why (or maybe a new src/backend/jit/llvm/README would be even better). The
> license doesn't have to be in a separate file AFAICT and including a (version
> of) your excellent summary in the commit message along with it would probably
> help readers.

Thank you. I figured that
src/backend/jit/llvm/SectionMemoryManager.LICENSE would be a good
place, to make it clear which code it covers and to remember to delete
it when the time comes. If we ever have to do more of this sort of
thing we might want to rename it to something more general, but I hope
not! I also updated the comments with a briefer summary of the points
from the commit message, at the top of the .cpp file. I added a
pgindent exclusion for the new alien header, or else it gets
scrambled.

I was worried for a while about the C++14 code in here (eg
std::make_unique), something we've avoided using in the past, but
after a bit of 3D versionography I'm pretty sure there is no issue and
we don't have to contort the code to avoid it.

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.)

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+.

Attachment Content-Type Size
v8-0001-Monkey-patch-LLVM-code-to-fix-ARM-relocation-bug.patch application/x-patch 50.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Sutou Kouhei 2024-11-05 08:43:28 Re: Make COPY format extendable: Extract COPY TO format implementations
Previous Message Amit Langote 2024-11-05 07:55:25 Re: Wrong result when enable_partitionwise_join is on if collation of PartitionKey and Column is different.