Thread: seawasp failing, maybe in glibc allocator
Hi, Since seawasp's bleeding-edge clang moved to "20210226", it failed every run except 4, and a couple of days ago it moved to "20210508" and it's still broken. It's always like this: 2021-05-09 03:31:37.602 CEST [1678796:171] pg_regress/_int LOG: statement: RESET enable_seqscan; corrupted double-linked list ... which doesn't appear in our code, but matches this: https://github.com/bminor/glibc/blob/cedbf6d5f3f70ca911176de87d6e453eeab4b7a1/malloc/malloc.c#L1645 No reason to think it's our fault, but it'd be nice to see a backtrace. Is gdb installed, and are core files being dumped by that SIGABRT, and are they using the default name (/proc/sys/kernel/core_pattern = core), which the BF can find with the value it's using, namely 'core_file_glob' => 'core*'?
Hello Thomas, > Since seawasp's bleeding-edge clang moved to "20210226", it failed > every run except 4, and a couple of days ago it moved to "20210508" > and it's still broken. Indeed I have noticed that there is indeed an issue, but the investigation is not very high on my current too deep pg-unrelated todo list. > It's always like this: > > 2021-05-09 03:31:37.602 CEST [1678796:171] pg_regress/_int LOG: > statement: RESET enable_seqscan; > corrupted double-linked list > > ... which doesn't appear in our code, but matches this: > > https://github.com/bminor/glibc/blob/cedbf6d5f3f70ca911176de87d6e453eeab4b7a1/malloc/malloc.c#L1645 > No reason to think it's our fault, but it'd be nice to see a > backtrace. ISTM it looks like some kind of memory corruption. If I'd have to guess between glibc, clang and pg, not sure which one I'd chose between the two laters potential bug sources. > Is gdb installed, and are core files being dumped by that SIGABRT, and > are they using the default name (/proc/sys/kernel/core_pattern = core), > which the BF can find with the value it's using, namely 'core_file_glob' > => 'core*'? Nope: sh> cat /proc/sys/kernel/core_pattern |/usr/share/apport/apport %p %s %c %d %P %E -- Fabien.
On Mon, May 10, 2021 at 6:59 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > Is gdb installed, and are core files being dumped by that SIGABRT, and > > are they using the default name (/proc/sys/kernel/core_pattern = core), > > which the BF can find with the value it's using, namely 'core_file_glob' > > => 'core*'? > > Nope: > > sh> cat /proc/sys/kernel/core_pattern > |/usr/share/apport/apport %p %s %c %d %P %E If you don't care about Ubuntu "apport" on this system (something for sending crash/bug reports to developers with a GUI), you could uninstall it (otherwise it overwrites the core_pattern every time it restarts, no matter what you write in your sysctl.conf, apparently), and then sudo sysctl -w kernel.core_pattern=core to undo the setting immediately (or reboot). Then hopefully the build farm would succeed in dumping a backtrace into the log.
> If you don't care about Ubuntu "apport" on this system (something for > sending crash/bug reports to developers with a GUI), you could > uninstall it (otherwise it overwrites the core_pattern every time it > restarts, no matter what you write in your sysctl.conf, apparently), > and then sudo sysctl -w kernel.core_pattern=core to undo the setting > immediately (or reboot). Then hopefully the build farm would succeed > in dumping a backtrace into the log. I forced-removed apport (which meant removing xserver-xorg). Let's see whether the reports are better or whether I break something. -- Fabien.
On Mon, May 10, 2021 at 9:30 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > I forced-removed apport (which meant removing xserver-xorg). Let's see > whether the reports are better or whether I break something. And of course this time it succeeded :-) Just by the way, I noticed it takes ~40 minutes to compile. Is there a reason you don't install ccache and set eg CC="ccache /path/to/clang", CXX="ccache /path/to/clang++", CLANG="ccache /path/to/clang"? That doesn't seem to conflict with your goal of testing LLVM/Clang's main branch, because ccache checks the compiler's mtime as part of its cache invalidation strategy.
> And of course this time it succeeded :-) Hmmm. ISTM that failures are on and off every few attempts. > Just by the way, I noticed it takes ~40 minutes to compile. Is there > a reason you don't install ccache and set eg CC="ccache > /path/to/clang", CXX="ccache /path/to/clang++", CLANG="ccache > /path/to/clang"? That doesn't seem to conflict with your goal of > testing LLVM/Clang's main branch, because ccache checks the compiler's > mtime as part of its cache invalidation strategy. Yep. I remember that I disactivated it for some reason once, but I cannot remember why. I just reactivated it, will see what happens. -- Fabien.
On Mon, May 10, 2021 at 11:21 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > And of course this time it succeeded :-) > > Hmmm. ISTM that failures are on and off every few attempts. OK we got the SIGABRT this time, but still no backtrace. If the kernel's core_pattern is "core", gdb is installed, then considering that the buildfarm core_file_glob is "core*" and the script version is recent (REL_12), then I'm out of ideas. ulimit -c XXX shouldn't be needed because the perl script does that with rlimit.
On 2021-05-11 12:16:44 +1200, Thomas Munro wrote: > OK we got the SIGABRT this time, but still no backtrace. If the > kernel's core_pattern is "core", gdb is installed, then considering > that the buildfarm core_file_glob is "core*" and the script version is > recent (REL_12), then I'm out of ideas. ulimit -c XXX shouldn't be > needed because the perl script does that with rlimit. Unless perhaps the hard rlimit for -C is set? ulimit -c -H should show that.
> On 2021-05-11 12:16:44 +1200, Thomas Munro wrote: >> OK we got the SIGABRT this time, but still no backtrace. If the >> kernel's core_pattern is "core", gdb is installed, then considering >> that the buildfarm core_file_glob is "core*" and the script version is >> recent (REL_12), then I'm out of ideas. ulimit -c XXX shouldn't be >> needed because the perl script does that with rlimit. > > Unless perhaps the hard rlimit for -C is set? ulimit -c -H should show > that. Possibly I have just added "ulimit -c unlimited" in the script, we should see the effect on next round. -- Fabien.
On 2021-05-11 10:22:02 +0200, Fabien COELHO wrote: > > > On 2021-05-11 12:16:44 +1200, Thomas Munro wrote: > > > OK we got the SIGABRT this time, but still no backtrace. If the > > > kernel's core_pattern is "core", gdb is installed, then considering > > > that the buildfarm core_file_glob is "core*" and the script version is > > > recent (REL_12), then I'm out of ideas. ulimit -c XXX shouldn't be > > > needed because the perl script does that with rlimit. > > > > Unless perhaps the hard rlimit for -C is set? ulimit -c -H should show > > that. > > Possibly I have just added "ulimit -c unlimited" in the script, we should > see the effect on next round. If it's the hard limit that won't help, because the hard limit can only be increased by a privileged process.
Hello Andres,p >>> Unless perhaps the hard rlimit for -C is set? ulimit -c -H should show >>> that. >> >> Possibly I have just added "ulimit -c unlimited" in the script, we should >> see the effect on next round. > > If it's the hard limit that won't help, because the hard limit can only > be increased by a privileged process. It seems to be the soft one, so ISTM that it may work. Before: sh> ulimit -c -S 0 sh> ulimit -c -H unlimited Then after: sh> ulimit -c unlimited I have: sh> ulimit -c -S unlimited sh> ulimit -c -H unlimited -- Fabien.
> Possibly I have just added "ulimit -c unlimited" in the script, we should see
> the effect on next round.
for def5b065 it ended on on the contrib ltree test:
2021-05-12 20:12:52.528 CEST [3042602:410] pg_regress/ltree LOG: disconnection: session time: 0:00:13.426
user=buildfarm database=contrib_regression_ltree host=[local]
/home/fabien/pg/build-farm-12/buildroot/HEAD/pgsql.build/contrib/ltree/results/ltree.out
2021-05-12 20:12:52.523330311 +0200
@@ -7931,11 +7931,8 @@
(1 row)
SELECT count(*) FROM _ltreetest WHERE t ~ '1.1.1.*' ;
- count
--------
- 19
-(1 row)
-
+ERROR: stack depth limit exceeded
+HINT: Increase the configuration parameter "max_stack_depth" (currently 2048kB), after ensuring the platform's
stackdepth limit is adequate.
SELECT count(*) FROM _ltreetest WHERE t ~ '*.1' ;
count
-------
Not a core dump, though.
--
Fabien.
Hello Andres, It finally failed with a core on 8f72bba, in llvm_shutdown, AFAIKS in a free while doing malloc-related housekeeping. My guess is that there is an actual memory corruption somewhere. It is unobvious whether it is in bleeding-edge llvm or bleeding-edge postgres though. The issue is non-deterministically triggered in contrib checks, either in int or ltree, but not elsewhere. This suggests issues specific to these modules, or triggered by these modules. Hmmm… I've just launched a run with valgrind enabled. -- Fabien.
On Sat, May 15, 2021 at 6:41 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > The issue is non-deterministically triggered in contrib checks, either in > int or ltree, but not elsewhere. This suggests issues specific to these > modules, or triggered by these modules. Hmmm… Hmm, yeah. A couple of different ways that ltreetest fails without crashing: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-05-13%2001%3A17%3A15 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-05-12%2017%3A17%3A15 Otherwise it's mostly free() blowing up, and one case of an assertion failure in llvm::StringMapImpl::RemoveKey, I guess before free() is reached: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-05-14%2009%3A17%3A15
>> The issue is non-deterministically triggered in contrib checks, either in >> int or ltree, but not elsewhere. This suggests issues specific to these >> modules, or triggered by these modules. Hmmm… > > Hmm, yeah. A couple of different ways that ltreetest fails without crashing: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-05-13%2001%3A17%3A15 > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-05-12%2017%3A17%3A15 > > Otherwise it's mostly free() blowing up, and one case of an assertion > failure in llvm::StringMapImpl::RemoveKey, I guess before free() is > reached: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-05-14%2009%3A17%3A15 It seems that the upload of the valgrind run (many hours…) failed on "413 request entity too large", and everything seems to have been cleaned despite the "--keepall" I think I put when I started the run. Not sure about the best way to proceed. -- Fabien.
On Wed, May 19, 2021 at 5:02 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > It seems that the upload of the valgrind run (many hours…) failed on "413 > request entity too large", and everything seems to have been cleaned > despite the "--keepall" I think I put when I started the run. I installed Clang/LLVM version "1:13~++20210520071732+02f2d739e074-1~exp1~20210520052519.57" from https://apt.llvm.org/ on a Debian buster box, and I saw that contrib/ltree's test fail about half the time with a range of weird and wonderful outputs (wrong answers) similar to seawasp, but it never crashed. I ran it under valgrind and I managed to get: ==529250== Invalid read of size 8 ==529250== at 0x1475B6CA: void std::vector<llvm::orc::SymbolStringPtr, std::allocator<llvm::orc::SymbolStringPtr> >::_M_realloc_insert<llvm::orc::SymbolStringPtr const&>(__gnu_cxx::__normal_iterator<llvm::orc::SymbolStringPtr*, std::vector<llvm::orc::SymbolStringPtr, std::allocator<llvm::orc::SymbolStringPtr> > >, llvm::orc::SymbolStringPtr const&) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x1474F246: llvm::orc::JITDylib::removeTracker(llvm::orc::ResourceTracker&) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x14741E0A: llvm::orc::ExecutionSession::removeResourceTracker(llvm::orc::ResourceTracker&) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x14747421: llvm::orc::JITDylib::clear() (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x14751F5B: llvm::orc::ExecutionSession::endSession() (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x14785207: llvm::orc::LLJIT::~LLJIT() (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x147A4C7D: LLVMOrcDisposeLLJIT (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x124D0094: llvm_shutdown (llvmjit.c:892) ==529250== by 0x57BC08: proc_exit_prepare (ipc.c:209) ==529250== by 0x57BC77: proc_exit (ipc.c:107) ==529250== by 0x5A545B: PostgresMain (postgres.c:4700) ==529250== by 0x517569: BackendRun (postmaster.c:4491) ==529250== by 0x517569: BackendStartup (postmaster.c:4213) ==529250== by 0x517569: ServerLoop (postmaster.c:1745) ==529250== Address 0x1a969088 is 1,416 bytes inside a block of size 3,072 free'd ==529250== at 0x4839EAB: operator delete(void*) (vg_replace_malloc.c:584) ==529250== by 0x141DFD8E: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x141DD9C0: llvm::LazyValueInfo::releaseMemory() (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x13294832: llvm::PMDataManager::freePass(llvm::Pass*, llvm::StringRef, llvm::PassDebuggingString) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x13294774: llvm::PMDataManager::removeDeadPasses(llvm::Pass*, llvm::StringRef, llvm::PassDebuggingString) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x13290A69: llvm::FPPassManager::runOnFunction(llvm::Function&) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x14137632: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x13290FDE: llvm::legacy::PassManagerImpl::run(llvm::Module&) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x131F8625: LLVMRunPassManager (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x124D0772: llvm_optimize_module (llvmjit.c:620) ==529250== by 0x124D0772: llvm_compile_module (llvmjit.c:671) ==529250== by 0x124D0772: llvm_get_function (llvmjit.c:291) ==529250== by 0x124DA3BC: ExecRunCompiledExpr (llvmjit_expr.c:2402) ==529250== by 0x41D15C: ExecEvalExprSwitchContext (executor.h:339) ==529250== by 0x41D15C: ExecQual (executor.h:408) ==529250== by 0x41D15C: ExecScan (execScan.c:227) ==529250== Block was alloc'd at ==529250== at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342) ==529250== by 0x141E47D6: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x141E43B7: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x141E3202: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x141E0501: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x141DDD74: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x141DDC4C: llvm::LazyValueInfo::getConstant(llvm::Value*, llvm::Instruction*) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x13D0EE4B: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x13D11D42: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x1329098C: llvm::FPPassManager::runOnFunction(llvm::Function&) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x14137632: ??? (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== by 0x13290FDE: llvm::legacy::PassManagerImpl::run(llvm::Module&) (in /usr/lib/x86_64-linux-gnu/libLLVM-13.so.1) ==529250== Maybe they should rewrite LLVM in Rust.
Hi, On 2021-05-21 21:37:22 +1200, Thomas Munro wrote: > I installed Clang/LLVM version > "1:13~++20210520071732+02f2d739e074-1~exp1~20210520052519.57" from > https://apt.llvm.org/ on a Debian buster box, and I saw that > contrib/ltree's test fail about half the time with a range of weird > and wonderful outputs (wrong answers) similar to seawasp, but it never > crashed. I ran it under valgrind and I managed to get: Interesting. I tried this with a slightly older LLVM checkout (6f4f0afaa8ae), from 2021-04-20, contrib/ltree tests run without an issue, even if I force everything to be jitted+inlined+optimized. The git hash in the package version indicates the commit is from 2021-05-20. Upgrading my local checkout to see whether I can repro the problem. If I can we at least have a not too large bisection window... Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > Interesting. I tried this with a slightly older LLVM checkout > (6f4f0afaa8ae), from 2021-04-20, contrib/ltree tests run without an > issue, even if I force everything to be jitted+inlined+optimized. The > git hash in the package version indicates the commit is from > 2021-05-20. Upgrading my local checkout to see whether I can repro the > problem. If I can we at least have a not too large bisection window... We know that seawasp was okay as of configure: using compiler=clang version 13.0.0 (https://github.com/llvm/llvm-project.git f22d3813850f9e87c5204df6844a93b8c5db7730) and not okay as of configure: using compiler=clang version 13.0.0 (https://github.com/llvm/llvm-project.git 0e8f5e4a6864839d2292ec1ddfe48b6178c01f85) so that should correspond to a window of about a week, if I gather Fabien's update strategy correctly. regards, tom lane
Hi, On 2021-05-21 14:58:38 -0700, Andres Freund wrote: > Interesting. I tried this with a slightly older LLVM checkout > (6f4f0afaa8ae), from 2021-04-20, contrib/ltree tests run without an > issue, even if I force everything to be jitted+inlined+optimized. The > git hash in the package version indicates the commit is from > 2021-05-20. Upgrading my local checkout to see whether I can repro the > problem. If I can we at least have a not too large bisection window... After resolving some PEBKAC issues I was able to reproduce the issue after a git pull for fresh llvm sources and rebuilding. Hope I can narrow it down without needing to bisect, there's faster building projects than LLVM... Greetings, Andres Freund
Hi, On 2021-05-21 18:18:54 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Interesting. I tried this with a slightly older LLVM checkout > > (6f4f0afaa8ae), from 2021-04-20, contrib/ltree tests run without an > > issue, even if I force everything to be jitted+inlined+optimized. The > > git hash in the package version indicates the commit is from > > 2021-05-20. Upgrading my local checkout to see whether I can repro the > > problem. If I can we at least have a not too large bisection window... > > We know that seawasp was okay as of > > configure: using compiler=clang version 13.0.0 (https://github.com/llvm/llvm-project.git f22d3813850f9e87c5204df6844a93b8c5db7730) > > and not okay as of > > configure: using compiler=clang version 13.0.0 (https://github.com/llvm/llvm-project.git 0e8f5e4a6864839d2292ec1ddfe48b6178c01f85) > > so that should correspond to a window of about a week, if I gather > Fabien's update strategy correctly. I found the LLVM commit to blame (c8fc5e3ba942057d6c4cdcd1faeae69a28e7b671). Contacting the author and reading the change to see if I can spit the issue myself. Greetings, Andres Freund
Hi, On 2021-05-21 15:57:01 -0700, Andres Freund wrote: > I found the LLVM commit to blame (c8fc5e3ba942057d6c4cdcd1faeae69a28e7b671). > Contacting the author and reading the change to see if I can spit the > issue myself. Hrmpf. It's a silent API breakage. The author intended to email us about it, but apparently forgot. One now needs to increment a string-pool refcount. The reason that didn't trigger a reliable crash is that there's a path where the refcount of string-pool entries aren't asserted to be above before decrementing the refcount... And that there practically never are references to the pool entries after use. Continuing to discusss whether there's a better way to deal with this. Greetings, Andres Freund
> We know that seawasp was okay as of > > configure: using compiler=clang version 13.0.0 (https://github.com/llvm/llvm-project.git f22d3813850f9e87c5204df6844a93b8c5db7730) > > and not okay as of > > configure: using compiler=clang version 13.0.0 (https://github.com/llvm/llvm-project.git 0e8f5e4a6864839d2292ec1ddfe48b6178c01f85) > > so that should correspond to a window of about a week, if I gather > Fabien's update strategy correctly. Indeed, the full recompilation is triggered once a week, on Saturdays. If the build fails for some reason the previous version is kept and I may or may not have time to look at it and try to fix it for some time after that, or I may happen that I do not notice the issue for some time… -- Fabien.
On Sat, May 22, 2021 at 12:25 PM Andres Freund <andres@anarazel.de> wrote:
> On 2021-05-21 15:57:01 -0700, Andres Freund wrote:
> > I found the LLVM commit to blame (c8fc5e3ba942057d6c4cdcd1faeae69a28e7b671).
> > Contacting the author and reading the change to see if I can spit the
> > issue myself.
>
> Hrmpf. It's a silent API breakage. The author intended to email us about
> it, but apparently forgot. One now needs to increment a string-pool
> refcount. The reason that didn't trigger a reliable crash is that
> there's a path where the refcount of string-pool entries aren't asserted
> to be above before decrementing the refcount... And that there
> practically never are references to the pool entries after use.
>
> Continuing to discusss whether there's a better way to deal with this.
Any news?
FWIW this change appears to fix the problem for my system (LLVM 13
build from a couple of days ago). No more weird results, valgrind
errors gone. I ran the leak checker to see if I now had the opposite
problem, and although there are various leaks reported, I didn't see
obvious intern pool related stacks.
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 71029a39a9..7b09e520f5 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -1116,6 +1116,11 @@
llvm_resolve_symbols(LLVMOrcDefinitionGeneratorRef GeneratorObj, void
*Ctx,
if (error != LLVMErrorSuccess)
LLVMOrcDisposeMaterializationUnit(mu);
+#if LLVM_VERSION_MAJOR > 12
+ for (int i = 0; i < LookupSetSize; i++)
+ LLVMOrcRetainSymbolStringPoolEntry(symbols[i].Name);
+#endif
+
pfree(symbols);
return error;
On Sat, Jun 19, 2021 at 5:07 PM Thomas Munro <thomas.munro@gmail.com> wrote: > if (error != LLVMErrorSuccess) > LLVMOrcDisposeMaterializationUnit(mu); > > +#if LLVM_VERSION_MAJOR > 12 > + for (int i = 0; i < LookupSetSize; i++) > + LLVMOrcRetainSymbolStringPoolEntry(symbols[i].Name); > +#endif (Though, erm, that code probably either needs to move a bit further up or become conditional, considering the error case immediately above it, not sure which...)
Thomas Munro <thomas.munro@gmail.com> writes:
> On Sat, Jun 19, 2021 at 5:07 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>> if (error != LLVMErrorSuccess)
>> LLVMOrcDisposeMaterializationUnit(mu);
>>
>> +#if LLVM_VERSION_MAJOR > 12
>> + for (int i = 0; i < LookupSetSize; i++)
>> + LLVMOrcRetainSymbolStringPoolEntry(symbols[i].Name);
>> +#endif
> (Though, erm, that code probably either needs to move a bit further up
> or become conditional, considering the error case immediately above
> it, not sure which...)
Is a compile-time conditional really going to be reliable? See nearby
arguments about compile-time vs run-time checks for libpq features.
It's not clear to me how tightly LLVM binds its headers and running
code.
regards, tom lane
Hi, On 2021-06-19 17:07:51 +1200, Thomas Munro wrote: > On Sat, May 22, 2021 at 12:25 PM Andres Freund <andres@anarazel.de> wrote: > > On 2021-05-21 15:57:01 -0700, Andres Freund wrote: > > > I found the LLVM commit to blame (c8fc5e3ba942057d6c4cdcd1faeae69a28e7b671). > > > Contacting the author and reading the change to see if I can spit the > > > issue myself. > > > > Hrmpf. It's a silent API breakage. The author intended to email us about > > it, but apparently forgot. One now needs to increment a string-pool > > refcount. The reason that didn't trigger a reliable crash is that > > there's a path where the refcount of string-pool entries aren't asserted > > to be above before decrementing the refcount... And that there > > practically never are references to the pool entries after use. > > > > Continuing to discusss whether there's a better way to deal with this. > > Any news? Nothing really. I'd discussed it a bit with the relevant LLVM maintainer, but we didn't come to a real resolution. He apologized for not giving a proper heads up - he'd planned to send out an email, but somehow lost track of that. Given that any potential solution would be also end up being a versioned ifdef, I think adding something like what you suggest here is the least unreasonable solution. > FWIW this change appears to fix the problem for my system (LLVM 13 > build from a couple of days ago). No more weird results, valgrind > errors gone. I ran the leak checker to see if I now had the opposite > problem, and although there are various leaks reported, I didn't see > obvious intern pool related stacks. > > diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c > index 71029a39a9..7b09e520f5 100644 > --- a/src/backend/jit/llvm/llvmjit.c > +++ b/src/backend/jit/llvm/llvmjit.c > @@ -1116,6 +1116,11 @@ > llvm_resolve_symbols(LLVMOrcDefinitionGeneratorRef GeneratorObj, void > *Ctx, > if (error != LLVMErrorSuccess) > LLVMOrcDisposeMaterializationUnit(mu); > > +#if LLVM_VERSION_MAJOR > 12 > + for (int i = 0; i < LookupSetSize; i++) > + LLVMOrcRetainSymbolStringPoolEntry(symbols[i].Name); > +#endif > + > pfree(symbols); I think this should be part of the earlier loop? Once LLVMOrcAbsoluteSymbols() is called that owns the reference, so there doesn't seem to be a reason to increase the refcount only later? Greetings, Andres Freund
Hi, On 2021-06-19 10:12:03 -0400, Tom Lane wrote: > Is a compile-time conditional really going to be reliable? See nearby > arguments about compile-time vs run-time checks for libpq features. > It's not clear to me how tightly LLVM binds its headers and running > code. It should be fine (and if not we have plenty other places it'd be problematic). LLVM embeds the version between user of llvm and the library version in some symbol, so if there's a sufficient mismatch it'll cause link time issues. Of course that only works for major versions, but that shouldn't be an issue here. Greetings, Andres Freund
On Sun, Jun 20, 2021 at 10:59 PM Andres Freund <andres@anarazel.de> wrote: > I think this should be part of the earlier loop? Once > LLVMOrcAbsoluteSymbols() is called that owns the reference, so there > doesn't seem to be a reason to increase the refcount only later? Right, that makes sense. Here's a patch like that. Looking at their release schedule on https://llvm.org/, I see we have a gamble to make. They currently plan to cut RC1 at the end of July, and to release in late September (every second LLVM major release coincides approximately with a PG major release). Option 1: wait until we branch for 14, and then push this to master so that at least seawasp can get back to looking for new problems, and then back-patch only after they release (presumably in time for our November releases). If their API change sticks, PostgreSQL crashes and gives weird results with the initial release of LLVM 13 until our fix comes out. Option 2: get ahead of their release and get this into 14 + August back branch releases based on their current/RC behaviour. If they decide to revert the change before the final release, we'll leak symbol names because we hold an extra reference, until we can fix that. For the last round of changes[1], there was a similar when-to-act question, but that was a doesn't-compile-anymore API change, whereas this is a silent demons-might-fly-out-of-your-nose API change. [1] https://www.postgresql.org/message-id/flat/20201016011244.pmyvr3ee2gbzplq4%40alap3.anarazel.de
Attachment
Thomas Munro <thomas.munro@gmail.com> writes: > Looking at their release schedule on https://llvm.org/, I see we have > a gamble to make. They currently plan to cut RC1 at the end of July, > and to release in late September (every second LLVM major release > coincides approximately with a PG major release). Option 1: wait > until we branch for 14, and then push this to master so that at least > seawasp can get back to looking for new problems, and then back-patch > only after they release (presumably in time for our November > releases). If their API change sticks, PostgreSQL crashes and gives > weird results with the initial release of LLVM 13 until our fix comes > out. Option 2: get ahead of their release and get this into 14 + > August back branch releases based on their current/RC behaviour. If > they decide to revert the change before the final release, we'll leak > symbol names because we hold an extra reference, until we can fix > that. If that's an accurate characterization of the tradeoff, I have little difficulty in voting for #2. A crash is strictly worse than a memory leak. Besides which, I've heard little indication that they might revert. regards, tom lane
On Sun, Jun 20, 2021 at 11:01 PM Andres Freund <andres@anarazel.de> wrote: > On 2021-06-19 10:12:03 -0400, Tom Lane wrote: > > Is a compile-time conditional really going to be reliable? See nearby > > arguments about compile-time vs run-time checks for libpq features. > > It's not clear to me how tightly LLVM binds its headers and running > > code. > > It should be fine (and if not we have plenty other places it'd be > problematic). LLVM embeds the version between user of llvm and the > library version in some symbol, so if there's a sufficient mismatch > it'll cause link time issues. Of course that only works for major > versions, but that shouldn't be an issue here. I looked into this a bit. On the usual Unixoid server OSes the first line of defence is that the major version is baked into the library name to support parallel installation of major versions, so our llvmjit.so is linked against eg libLLVM-13.so.1 (all controlled by llvm-config), and then there are further defences like versioned symbols, LLVM_13 etc on some platforms. Curiously, they skip this scheme for Macs (see their AddLLVM.cmake file) and Windows. So of course I wanted to try to see if I could make it break in the way Tom imagined, on a Mac. There, I use MacPorts, and it has separate packages for major versions, for example "llvm-12", much like other distros. The package maintainers put libLLVM.dylib (LLVM project's choice for this platform) into different paths under .../libexec/llvm-$VERSION/.... (package maintainer's choice), and there is a tool to select the current default (alarm bells ringing at this point). The first observation is that the Mach-O "compatibility version" is 1.0.0 on all the .dylibs, so yeah, that mechanism isn't going to save you, but ... it turns out to be a moot question for now because, to my surprise, we're statically linking LLVM into our llvmjit.so on that platform. That turns out to be because llvm-config --libs won't spit out dynamic link options if it can't find a library name with the version embedded in it. I see now that Brew's maintainers take it on themselves to create that symlink[1] (unlike MacPorts'), so ... erm, could be trouble there, I dunno because I don't want to install that, but if so, maybe they asked for it? I guess that none of this stuff really matters for real world non-hacker users, who are probably using an installer that ships its own copy of the thing. I expect it'll be the same on Windows when we eventually support LLVM there. /me closes Macintosh [1] https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/llvm.rb#L175
Hi, On 2021-06-20 19:56:56 -0400, Tom Lane wrote: > Thomas Munro <thomas.munro@gmail.com> writes: > > Looking at their release schedule on https://llvm.org/, I see we have > > a gamble to make. They currently plan to cut RC1 at the end of July, > > and to release in late September (every second LLVM major release > > coincides approximately with a PG major release). Option 1: wait > > until we branch for 14, and then push this to master so that at least > > seawasp can get back to looking for new problems, and then back-patch > > only after they release (presumably in time for our November > > releases). If their API change sticks, PostgreSQL crashes and gives > > weird results with the initial release of LLVM 13 until our fix comes > > out. Option 2: get ahead of their release and get this into 14 + > > August back branch releases based on their current/RC behaviour. If > > they decide to revert the change before the final release, we'll leak > > symbol names because we hold an extra reference, until we can fix > > that. I think I'd vote for 2 or 2+ (backpatch immediately). > If that's an accurate characterization of the tradeoff, I have little > difficulty in voting for #2. A crash is strictly worse than a memory > leak. Besides which, I've heard little indication that they might > revert. We might be able to get them to revert and put in a different API, but I don't think it'd clearly be an improvement at this point. Greetings, Andres Freund
On Mon, Jun 21, 2021 at 11:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > If that's an accurate characterization of the tradeoff, I have little > difficulty in voting for #2. A crash is strictly worse than a memory > leak. Besides which, I've heard little indication that they might > revert. Agreed. On Mon, Jun 21, 2021 at 10:23 PM Andres Freund <andres@anarazel.de> wrote: > I think I'd vote for 2 or 2+ (backpatch immediately). Yeah, that makes sense. Done. Seawasp should turn green on its next run.
Hello Thomas, > Seawasp should turn green on its next run. Hopefully. It is not scheduled very soon because Tom complained about the induced noise in one buildfarm report, so I put the check to once a week. I changed it to start a run in a few minutes. I've rescheduled to once a day after that (previous schedule was a check every hour). -- Fabien.
>> Seawasp should turn green on its next run. It did! -- Fabien.