llvm::User::operator new(size_t) relies on undefined behaviour

[KrissN]

Bugzilla Link	48216
Version	10.0
OS	Linux
CC	@dwblaikie,@zygoloid

Extended Description

When switching my Gentoo system to GCC10 I have started seeing the following crash in clang:

clang -idirafter /usr/lib/llvm/10/bin/../../../../lib/clang/10.0.1/include -idirafter /usr/include -D__linux__ -I../../../include -target bpf -O2 -c grundig.c 
/usr/lib/llvm/10/bin/clangd
Stack dump:
0.      Program arguments: clang-10 -idirafter /usr/lib/llvm/10/bin/../../../../lib/clang/10.0.1/include -idirafter /usr/include -D__linux__ -I../../../include -target bpf -O2 -c grundig.c -o grundig.o
1.      <eof> parser at end of file
2.      Per-function optimization
3.      Running pass 'SROA' on function '@bpf_decoder'
#0 0x00007f61aea47b6a llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x787b6a)
#1 0x00007f61aea2b2f4 llvm::sys::RunSignalHandlers() (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x76b2f4)
#2 0x00007f61aea2b5b1 llvm::sys::CleanupOnSignal(unsigned long) (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x76b5b1)
#3 0x00007f61ae9a6758 (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x6e6758)
#4 0x00007f61b3fcbb70 (/lib64/libc.so.6+0x38b70)
#5 0x00007f61af21fb9f (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0xf5fb9f)
#6 0x00007f61af22189d llvm::PromoteMemToReg(llvm::ArrayRef<llvm::AllocaInst*>, llvm::DominatorTree&, llvm::AssumptionCache*) (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0xf6189d)
#7 0x00007f61af4e1f0f llvm::SROA::runImpl(llvm::Function&, llvm::DominatorTree&, llvm::AssumptionCache&) (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x1221f0f)
#8 0x00007f61af4e2c17 (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x1222c17)
#9 0x00007f61aeb448f3 llvm::FPPassManager::runOnFunction(llvm::Function&) (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x8848f3)
#10 0x00007f61aeb44b70 llvm::legacy::FunctionPassManagerImpl::run(llvm::Function&) (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x884b70)
#11 0x00007f61b2965dea clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, cla
ng::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x11b2dea)
#12 0x00007f61b2be46a9 (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x14316a9)
#13 0x00007f61b1ecfb8d clang::ParseAST(clang::Sema&, bool, bool) (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x71cb8d)
#14 0x00007f61b31ced11 clang::FrontendAction::Execute() (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x1a1bd11)
#15 0x00007f61b31847e7 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x19d17e7)
#16 0x00007f61b32151a6 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x1a621a6)
#17 0x000055b7cc2637e3 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/usr/lib/llvm/10/bin/clang-10+0x157e3)
#18 0x000055b7cc26a25f (/usr/lib/llvm/10/bin/clang-10+0x1c25f)
#19 0x00007f61b2ee57f5 (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x17327f5)
#20 0x00007f61ae995c53 llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/usr/lib/llvm/10/bin/../lib64/libLLVM-10.so+0x6d5c53)
#21 0x00007f61b2efaaae clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, bool*) const (/usr/lib/llvm/10/bin/../lib64/libcl
ang-cpp.so.10+0x1747aae)
#22 0x00007f61b2eefa56 clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&) const (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x173ca56)
#23 0x00007f61b2eefe05 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) const (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x173ce05)
#24 0x00007f61b2f0529c clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*> >&) (/usr/lib/llvm/10/bin/../lib64/libclang-cpp.so.10+0x175229c)
#25 0x000055b7cc25dda5 main (/usr/lib/llvm/10/bin/clang-10+0xfda5)
#26 0x00007f61b3fb6caa __libc_start_main (/lib64/libc.so.6+0x23caa)
#27 0x000055b7cc25ef1a _start (/usr/lib/llvm/10/bin/clang-10+0x10f1a)
clang-10: error: clang frontend command failed due to signal (use -v to see invocation) 
clang version 10.0.1
Target: bpf
Thread model: posix
InstalledDir: /usr/lib/llvm/10/bin
clang-10: note: diagnostic msg: PLEASE submit a bug report to  and include the crash backtrace, preprocessed source, and associated run script. 
clang-10: note: diagnostic msg: 

Analysing the problem I discovered, that the problem occurs only when compiling LLVM with GCC10[1], -fno-semantic-interposition and -flto.

I started to dig deeper into what is happening and discovered - what I believe to be the root cause.

When LLVM is being built with -fno-semantic-interposition and -flto using GCC10, the optimizer is free to inline calls to functions, even if these are public and exported from the shared object. This applies - among others - to llvm::User::opertator new called from llvm::LandingPadInst::Create. When this happens, the generated assembly does not include initialization of the values inside llvm::Value class (NumUserOperands, HasHungOffUses, etc.).

Looking at the code I have noticed that these fields are initialized inside the overloaded operator new(), which I believe is undefined behaviour (C++14, [class.cdtor]), as referencing any member of an object before the constructor starts execution is undefined behaviour.

The stand-alone version of llvm::User::operator new() contains initialization instructions for the llvm::Value object fields, as - looking at the operator new() alone - the compiler cannot assume that it will always be executed right before the constructor. Since without the -fno-semantic-interposition flag all calls to llvm::User::oprerator new() must go through the PLT, the fields are always initialized.

When however the -fno-semantic-interposition flag is used, the compiler is free to inline the overloaded operator new() and then discovers, that the code to initialize the object fields executed before the constructor has no effect and optimizes it out.

[1] Actually it can also be reproduced with <GCC-10 by additionally adding the -finline-functions flag, which - before GCC9 was not enabled at -O2, but is enabled starting with GCC10.

[dwblaikie]

Sounds plausible to me, though I don't know for sure (Richard Smith might be able to offer a more authoritative reading here)

I'd be slightly surprised if GCC was actually optimizing on the basis of this part of the spec, but could be.

As for fixing this - looks to me like it'd be a fairly invasive/verbose cleanup, moving and duplicating ctor parameters.

eg: (just adding a quick assertion to one of these User::operator new):

return new (NumOperands, DescriptorBytes) CallInst(Ty, Func, Args, Bundles, NameStr, InsertBefore);

So NumOperands and DescriptorBytes do still need to be passed to the op new (to allocate extra buffer space before or after the User itself) and then also to CallInst ctor in this case, to be used to initialize the NumUserOperands, HasHungOffUses, and HasDescriptor... - touching every single User construction will be a lot of code to change, I'd wager. Not impossible by any means - and mitigated somewhat by much of that construction being done via factory functions like CallInst::Create owing to the already somewhat awkward part of having to pass some things to the op new and some to the CallInst ctor itself in this case.

[zygoloid]

Sounds plausible to me, though I don't know for sure (Richard Smith might be
able to offer a more authoritative reading here)

Yes, it's not correct to rely on values stored into memory before an object's lifetime begins to still be there afterwards.

I'd be slightly surprised if GCC was actually optimizing on the basis of
this part of the spec, but could be.

I believe GCC really does assume that the entire storage for an object becomes uninitialized when its construction begins -- at least, I've seen it perform other similar optimizations that suggest it's making that assumption.

As for fixing this - looks to me like it'd be a fairly invasive/verbose
cleanup, moving and duplicating ctor parameters.

I wonder if we can move the storage for the fields that the operator new initializes (NumHungOffUses, NumUserOperands, HasDescriptor) outside the User object and into adjacent memory. That should avoid the problem in question -- but there may be good reasons why that's infeasible. If that is infeasible, replacing direct calls to new (...) ClassDerivedFromUser with use of a factory function might be a different fix that we could apply fairly mechanically.

[dwblaikie]

Sounds plausible to me, though I don't know for sure (Richard Smith might be
able to offer a more authoritative reading here)

Yes, it's not correct to rely on values stored into memory before an
object's lifetime begins to still be there afterwards.

Thanks for validating!

I'd be slightly surprised if GCC was actually optimizing on the basis of
this part of the spec, but could be.

I believe GCC really does assume that the entire storage for an object
becomes uninitialized when its construction begins -- at least, I've seen it
perform other similar optimizations that suggest it's making that assumption.

Neat/surprising!

As for fixing this - looks to me like it'd be a fairly invasive/verbose
cleanup, moving and duplicating ctor parameters.

I wonder if we can move the storage for the fields that the operator new
initializes (NumHungOffUses, NumUserOperands, HasDescriptor) outside
the User object and into adjacent memory.

Oh, that seems like a good idea to me. If I get a chance I'll look into doing it that way.

That should avoid the problem in
question -- but there may be good reasons why that's infeasible. If that is
infeasible, replacing direct calls to new (...) ClassDerivedFromUser with
use of a factory function might be a different fix that we could apply
fairly mechanically.

I think most direct calls are through a factory - but there's a lot of derived classes & thus lots of factory functions that would need updating. Unless there's something more mechanical you've got in mind.