[urgent][CodeGen] First check the kind and then the llvm::Function properties.

[v.g.vassilev]

Summary

This patch fixes valgrind reports from downstream consumers about conditional jump over uninitialised.

[ RUN      ] ScopeReflectionTest.IsComplete
==987150== Conditional jump or move depends on uninitialised value(s)
==987150==    at 0x1E1128F: clang::CodeGen::CodeGenModule::SetLLVMFunctionAttributesForDefinition(clang::Decl const*, llvm::Function*) (CodeGenModule.cpp:2391)
==987150==    by 0x1E4F181: clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) (CodeGenModule.cpp:5669)
==987150==    by 0x1E4A194: clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) (CodeGenModule.cpp:3909)
==987150==    by 0x1E4A752: clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) (CodeGenModule.cpp:3649)
==987150==    by 0x1E532F5: clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) [clone .part.0] (CodeGenModule.cpp:6563)
==987150==    by 0x1B0BEDD: (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) (ModuleBuilder.cpp:190)
==987150==    by 0x1AEA47B: clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef) (CodeGenAction.cpp:235)
==987150==    by 0x101B02F: clang::IncrementalASTConsumer::HandleTopLevelDecl(clang::DeclGroupRef) (IncrementalParser.cpp:52)
==987150==    by 0x101ED93: clang::IncrementalParser::ParseOrWrapTopLevelDecl() (IncrementalParser.cpp:276)
==987150==    by 0x101FBBC: clang::IncrementalParser::Parse(llvm::StringRef) (IncrementalParser.cpp:342)
==987150==    by 0x100E104: clang::Interpreter::Parse(llvm::StringRef) (Interpreter.cpp:360)
==987150==    by 0xE734C0: Cpp::Interpreter::Parse(llvm::StringRef) (CppInterOpInterpreter.h:172)
==987150==  Uninitialised value was created by a heap allocation
==987150==    at 0x844BE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==987150==    by 0x1B0C882: StartModule (ModuleBuilder.cpp:139)
==987150==    by 0x1B0C882: clang::CodeGenerator::StartModule(llvm::StringRef, llvm::LLVMContext&) (ModuleBuilder.cpp:360)
==987150==    by 0x101C4AF: clang::IncrementalParser::GenModule() (IncrementalParser.cpp:372)
==987150==    by 0x101FC0E: clang::IncrementalParser::Parse(llvm::StringRef) (IncrementalParser.cpp:362)
==987150==    by 0x100E104: clang::Interpreter::Parse(llvm::StringRef) (Interpreter.cpp:360)
==987150==    by 0x100E243: clang::Interpreter::create(std::unique_ptr<clang::CompilerInstance, std::default_delete<clang::CompilerInstance> >) (Interpreter.cpp:279)
==987150==    by 0xF2131A: compat::createClangInterpreter(std::vector<char const*, std::allocator<char const*> >&) (Compatibility.h:123)
==987150==    by 0xF22AB9: Cpp::Interpreter::Interpreter(int, char const* const*, char const*, std::vector<std::shared_ptr<clang::ModuleFileExtension>, std::allocator<std::shared_ptr<clang::ModuleFileExtension> > > const&, void*, bool) (CppInterOpInterpreter.h:146)
==987150==    by 0xF1827A: CreateInterpreter (CppInterOp.cpp:2494)
==987150==    by 0xECFA0E: TestUtils::GetAllTopLevelDecls(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<clang::Decl*, std::allocator<clang::Decl*> >&, bool) (Utils.cpp:23)
==987150==    by 0xE9CB85: ScopeReflectionTest_IsComplete_Test::TestBody() (ScopeReflectionTest.cpp:71)
==987150==    by 0xF0ED0C: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /home/vvassilev/workspace/builds/scratch/cppyy/InterOp/build-with-clang-repl-release/unittests/CppInterOp/CppInterOpTests)
==987150==

[daltenty]

clang/lib/CodeGen/CodeGenModule.cpp

2404

Thanks for looking into this.

It's not clear to me how this re-ordering ends up fixing things. Can you clarify what the uninitialized value was in this expression?

[v.g.vassilev]

clang/lib/CodeGen/CodeGenModule.cpp

2404

The issue happens only in Release builds (RelWithDebInfo, too). From what I was able to see it is somewhere in F->getPointerAlignment. My assumption was that we cannot rely on the full properties of F to be set unless it is the declaration kind we expected (similar to checking if a something is a nullptr and then probing its members). Secondly the isa check is likely to be the less expensive check anyway.

I saw the issue yesterday and dug into it for a while. However, I decided to insert a "fix" before the release which is in few days since the isa seems to the faster check anyway.

[daltenty]

LGTM as a workaround.

clang/lib/CodeGen/CodeGenModule.cpp

2404

Thanks, yeah thats kind of what I expected. F->getPointerAlignment() is likely getting inlined into in to this callsite and we are inspecting uninitialized properties of the DataLayout. The weird part is I don't see why those properties of the DataLayout ever should be uninitialized, so I think there might be something more broken underneath this.

That said, this is definitely better than before as you say, so let's go ahead with this for the release and maybe I'll do some more digging in getPointerAlignment.

[v.g.vassilev]

clang/lib/CodeGen/CodeGenModule.cpp

2404

Sounds good. For better or worse that workaround might make the issue more subtle to debug next time it appears...

[v.g.vassilev]

The issue goes away for non-CXXMethodDecls but still persists in the case where we process:

CXXDestructorDecl 0x9ab0f48 </usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr.h:121:11> col:11 implicit used ~shared_ptr 'void () noexcept' inline default
`-CompoundStmt 0x9ac3ca0 <col:11>

Here is a bt:

#0  llvm::Value::getPointerAlignment (this=0x9dcbed8, DL=...) at /home/vvassilev/workspace/sources/llvm-project/llvm/lib/IR/Value.cpp:923
#1  0x0000000001b32b52 in clang::CodeGen::CodeGenModule::SetLLVMFunctionAttributesForDefinition (this=0x885e3c0, D=0x9ab0f48, F=0x9dcbed8)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:2389
#2  0x0000000001d40899 in clang::CodeGen::CodeGenModule::codegenCXXStructor (this=0x885e3c0, GD=...)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/CGCXX.cpp:211
#3  0x0000000001bbffd5 in (anonymous namespace)::ItaniumCXXABI::emitCXXStructor (this=0x885f2f0, GD=...)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/ItaniumCXXABI.cpp:4453
#4  0x0000000001b36ffb in clang::CodeGen::CodeGenModule::EmitGlobalDefinition (this=this@entry=0x885e3c0, GD=..., GV=GV@entry=0x9dcbed8)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:3893
#5  0x0000000001b2a9a0 in clang::CodeGen::CodeGenModule::EmitDeferred (this=this@entry=0x885e3c0)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:3050
#6  0x0000000001b2a9cc in clang::CodeGen::CodeGenModule::EmitDeferred (this=this@entry=0x885e3c0)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:3056
#7  0x0000000001b285bb in clang::CodeGen::CodeGenModule::Release (this=0x885e3c0)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:759
#8  0x0000000001882908 in (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit (this=0x87c6430, Ctx=...)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/ModuleBuilder.cpp:287
#9  0x000000000186e3a6 in clang::BackendConsumer::HandleTranslationUnit (this=0x87c6080, C=...)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:311
#10 0x0000000000f7bdc7 in clang::IncrementalParser::ParseOrWrapTopLevelDecl (this=this@entry=0x87603c0)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/Interpreter/IncrementalParser.cpp:303
#11 0x0000000000f7c760 in clang::IncrementalParser::Parse (this=0x87603c0, input=...)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/Interpreter/IncrementalParser.cpp:342
#12 0x0000000000f70c79 in clang::Interpreter::Parse (this=0x875d170, Code=...)
    at /home/vvassilev/workspace/sources/llvm-project/clang/lib/Interpreter/Interpreter.cpp:360
#13 0x0000000000e28b11 in Cpp::Interpreter::Parse (this=0x86e9cf0, Code=...)
    at /home/vvassilev/workspace/builds/scratch/cppyy/InterOp/include/clang/Interpreter/CppInterOpInterpreter.h:172
#14 0x0000000000e28b84 in Cpp::Interpreter::declare (this=0x86e9cf0, 
    input="\n    #include <memory>\n\n    template<typename T>\n    class derived_shared_ptr : public std::shared_ptr<T> {};\n    template<typename T>\n    class derived_unique_ptr : public std::unique_ptr<T> {};\n\n   "..., PTU=0x0)

I am staring at the Value::getPointerAlignment(const DataLayout &DL) and cannot find out what might be wrong. One particularity of the clang-repl based workflows is that it calls multiple times StartModule, which creates a new llvm::Module per partial translation unit.

cc: @Hahnfeld

[daltenty]

FYI: I tried to reproduce this on powerpc64le-linux-gnu at -02 and didn't see it at this point, though I get a whole lot of other uninitialized reads, so there's definitely some general problems in this space in the LLVM codebase

[v.g.vassilev]

In D159339#4642147, @daltenty wrote:

FYI: I tried to reproduce this on powerpc64le-linux-gnu at -02 and didn't see it at this point, though I get a whole lot of other uninitialized reads, so there's definitely some general problems in this space in the LLVM codebase

Are these also present in llvm16?

[daltenty]

In D159339#4642210, @v.g.vassilev wrote:

In D159339#4642147, @daltenty wrote:

FYI: I tried to reproduce this on powerpc64le-linux-gnu at -02 and didn't see it at this point, though I get a whole lot of other uninitialized reads, so there's definitely some general problems in this space in the LLVM codebase

Are these also present in llvm16?

I do see a similar case in llvm16 yes:

valgrind --tool=memcheck --leak-check=full ./bin/clang-repl
==3583806== Memcheck, a memory error detector
==3583806== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3583806== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==3583806== Command: ./bin/clang-repl
==3583806==
clang-repl> #include <memory>
==3583806== Conditional jump or move depends on uninitialised value(s)
==3583806==    at 0x28D08F4: operator== (llvm-project/llvm/include/llvm/Support/Alignment.h:301)
==3583806==    by 0x28D08F4: llvm::DataLayout::operator==(llvm::DataLayout const&) const (llvm-project/llvm/lib/IR/DataLayout.cpp:549)
==3583806==    by 0x2AA31BB: operator!= (llvm-project/llvm/include/llvm/IR/DataLayout.h:233)
==3583806==    by 0x2AA31BB: llvm::orc::LLJIT::applyDataLayout(llvm::Module&) (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:927)
==3583806==    by 0x2AA009F: operator() (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:754)
==3583806==    by 0x2AA009F: withModuleDo<(lambda at /home/daltenty/llvm/dev/llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:754:28)> (llvm-project/llvm/include/llvm/ExecutionEngine/Orc/ThreadSafeModule.h:136)
==3583806==    by 0x2AA009F: llvm::orc::LLJIT::addIRModule(llvm::IntrusiveRefCntPtr<llvm::orc::ResourceTracker>, llvm::orc::ThreadSafeModule) (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:754)
==3583806==    by 0x2AA0693: llvm::orc::LLJIT::addIRModule(llvm::orc::JITDylib&, llvm::orc::ThreadSafeModule) (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:761)
==3583806==    by 0x2AAB563: (anonymous namespace)::GenericLLVMIRPlatformSupport::setupJITDylib(llvm::orc::JITDylib&) (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:253)
==3583806==    by 0x2AA2B2F: GenericLLVMIRPlatformSupport (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:200)
==3583806==    by 0x2AA2B2F: make_unique<(anonymous namespace)::GenericLLVMIRPlatformSupport, llvm::orc::LLJIT &> (unique_ptr.h:1065)
==3583806==    by 0x2AA2B2F: llvm::orc::setUpGenericLLVMIRPlatform(llvm::orc::LLJIT&) (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:947)
==3583806==    by 0x2AA2273: llvm::orc::LLJIT::LLJIT(llvm::orc::LLJITBuilderState&, llvm::Error&) (llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:911)
==3583806==    by 0x26AE233: llvm::orc::LLJITBuilderSetters<llvm::orc::LLJIT, llvm::orc::LLJITBuilder, llvm::orc::LLJITBuilderState>::create() (llvm-project/llvm/include/llvm/ExecutionEngine/Orc/LLJIT.h:381)
==3583806==    by 0x2F39D7F: clang::IncrementalExecutor::IncrementalExecutor(llvm::orc::ThreadSafeContext&, llvm::Error&, clang::TargetInfo const&) (llvm-project/clang/lib/Interpreter/IncrementalExecutor.cpp:40)
==3583806==    by 0x2F33A47: make_unique<clang::IncrementalExecutor, llvm::orc::ThreadSafeContext &, llvm::Error &, const clang::TargetInfo &> (unique_ptr.h:1065)
==3583806==    by 0x2F33A47: clang::Interpreter::Execute(clang::PartialTranslationUnit&) (llvm-project/clang/lib/Interpreter/Interpreter.cpp:223)
==3583806==    by 0x26AE71B: clang::Interpreter::ParseAndExecute(llvm::StringRef) (llvm-project/clang/include/clang/Interpreter/Interpreter.h:68)
==3583806==    by 0x26ADD6B: main (llvm-project/clang/tools/clang-repl/ClangRepl.cpp:127)
==3583806==

This definitely seems to be point to DataLayout.FunctionPtrAlign.

Edit: this result was at -O2, and I get completely different traces at -O1 and lower, so I'm starting to think these traces are likely inaccurate

(Valgrind warns against running at -O2 or higher may lead to spurious uninitialized values in the manual)

clang/lib/CodeGen/CodeGenModule.cpp

if (!D->hasAttr<AlignedAttr>())
     if (LangOpts.FunctionAlignment)
       F->setAlignment(llvm::Align(1ull << LangOpts.FunctionAlignment));
 
   // Some C++ ABIs require 2-byte alignment for member functions, in order to
   // reserve a bit for differentiating between virtual and non-virtual member
   // functions. If the current target's C++ ABI requires this and this is a
   // member function, set its alignment accordingly.
   if (getTarget().getCXXABI().areMemberFunctionsAligned()) {
-    if (F->getPointerAlignment(getDataLayout()) < 2 && isa<CXXMethodDecl>(D))
+    if (isa<CXXMethodDecl>(D) && F->getPointerAlignment(getDataLayout()) < 2)
       F->setAlignment(std::max(llvm::Align(2), F->getAlign().valueOrOne()));
   }
 
   // In the cross-dso CFI mode with canonical jump tables, we want !type
   // attributes on definitions only.
   if (CodeGenOpts.SanitizeCfiCrossDso &&
       CodeGenOpts.SanitizeCfiCanonicalJumpTables) {
     if (auto *FD = dyn_cast<FunctionDecl>(D)) {