Correctly emit dwoIDs after ASTFileSignature refactoring (D81347)

[teemperor]

Summary

D81347 changes the ASTFileSignature to be an array of 20 uint8_t instead of 5 uint32_t.
However, it didn't update the code in ObjectFilePCHContainerOperations that creates
the dwoID in the module from the ASTFileSignature (Buffer->Signature being the
array subclass that is now std::array<uint8_t, 20> instead of std::array<uint32_t, 5>).

uint64_t Signature = [..] (uint64_t)Buffer->Signature[1] << 32 | Buffer->Signature[0]

This code works with the old ASTFileSignature (where two uint32_t are enough to
fill the uint64_t), but after the patch this only took two bytes from the ASTFileSignature
and only partly filled the Signature uint64_t.

This caused that the dwoID in the module ref and the dwoID in the actual module no
longer match (which in turns causes that LLDB keeps warning about the dwoID's not
matching when debugging -gmodules-compiled binaries).

This patch just unifies the logic for turning the ASTFileSignature into an uint64_t which
makes the dwoID match again (and should prevent issues like that in the future).

[dang]

Sorry about that. LGTM once you make a decision on my comments.

clang/include/clang/Basic/Module.h

67

Mega-nit: Not sure Id is the best name here, maybe TruncatedValue would be clearer.

68

Not sure you need std::min here, sizeof(*this) is definitely going to be larger than one uint64_t since it needs to store 20 bytes and is not going to get smaller than 8 bytes for sure. I don't care super strongly though if you think this cleaner.

[teemperor]

• Replaced min call with static_assert.
• Fixed variable name.

[teemperor]

In D84013#2157961, @dang wrote:

Sorry about that. LGTM once you make a decision on my comments.

No worries, thanks for refactoring this!

clang/include/clang/Basic/Module.h

67

Thanks, a leftover from pre-review refactoring :)

68

That was just about preventing OOB issues in case people change the signature size downwards (for whatever reason). I guess we could also just drop this logic if that ever happens, so I just made it a static_assert.

[dang]

clang/include/clang/Basic/Module.h

68

Nice!

[aprantl]

clang/include/clang/Basic/Module.h

70

is this the same as memcpy(&Value, data(), sizeof(Value)) or is there some subtle endianness issue I'm missing? That would seems easier to read to me.

clang/test/Modules/ModuleDebugInfoDwoId.cpp

4

I think this is redundant.

5

Since we just deleted %t, -I %t seems unnecessary?
For clarity, what do you think about -fmodules-cache-path=%t.cache

6

I think I'd rather run FileCheck on both files separately?

[dang]

clang/include/clang/Basic/Module.h

70

Yes that's right. Doing this via memcpy leads to different values depending on endianness. Essentially the signature is stored in little-endian byte order. If you just memcpy on a big-endian system, you would wind-up with the bytes swapped (the first byte of the array would be the MSB instead of the LSB). If you do this loop instead, the endianness is preserved (the first byte is always the LSB).

[teemperor]

clang/test/Modules/ModuleDebugInfoDwoId.cpp

4

That's actually deleting the module cache (if we don't do this, then the test won't rebuild the module and we don't get t.mod-out)

5

I like %t.cache much better. I admit the %t cache was actually just cargo-culted from the other Module/ tests :)

6

I can do that for CHECK-REALIDS, but the default check is comparing the dwoIDs and make sure they're equal, so I don't know if there is some magic trick to do that with two FileCheck runs.

[teemperor]

• %t -> %t.cache
• Don't concat out files and instead run FileCheck on each file where possible.

[teemperor]

Somehow this ended up failing on Fuchsia with a plain "Exit code 1", but not sure what's the fault. I'll revert until I figured that out.

[davezarzycki]

I have an auto-bisecting cron job that has identified the "reland" as breaking the test suite on Fedora 32 (x86). Is there a quick fix or can we revert the reland?

FAIL: Clang :: Modules/ModuleDebugInfoDwoId.cpp (12657 of 68968)
******************** TEST 'Clang :: Modules/ModuleDebugInfoDwoId.cpp' FAILED ********************
Script:
--
: 'RUN: at line 3';   rm -rf /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache
: 'RUN: at line 4';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-unknown-linux-gnu -x objective-c++ -std=c++11 -debugger-tuning=lldb -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs -fimplicit-module-maps -fmodules-cache-path=/tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp -I /home/dave/ro_s/lp/clang/test/Modules/Inputs -emit-llvm -o /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll -mllvm -debug-only=pchcontainer 2> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out
: 'RUN: at line 5';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll > /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output
: 'RUN: at line 6';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out >> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output
: 'RUN: at line 7';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
: 'RUN: at line 8';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll | /tmp/_update_lc/r/bin/FileCheck --check-prefix=CHECK-REALIDS /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
: 'RUN: at line 9';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out | /tmp/_update_lc/r/bin/FileCheck --check-prefix=CHECK-REALIDS /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
--
Exit Code: 1


********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (1):
  Clang :: Modules/ModuleDebugInfoDwoId.cpp

[teemperor]

In D84013#2233242, @davezarzycki wrote:

I have an auto-bisecting cron job that has identified the "reland" as breaking the test suite on Fedora 32 (x86). Is there a quick fix or can we revert the reland?

FAIL: Clang :: Modules/ModuleDebugInfoDwoId.cpp (12657 of 68968)
******************** TEST 'Clang :: Modules/ModuleDebugInfoDwoId.cpp' FAILED ********************
Script:
--
: 'RUN: at line 3';   rm -rf /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache
: 'RUN: at line 4';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-unknown-linux-gnu -x objective-c++ -std=c++11 -debugger-tuning=lldb -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs -fimplicit-module-maps -fmodules-cache-path=/tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp -I /home/dave/ro_s/lp/clang/test/Modules/Inputs -emit-llvm -o /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll -mllvm -debug-only=pchcontainer 2> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out
: 'RUN: at line 5';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll > /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output
: 'RUN: at line 6';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out >> /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output
: 'RUN: at line 7';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.combined_output | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
: 'RUN: at line 8';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.ll | /tmp/_update_lc/r/bin/FileCheck --check-prefix=CHECK-REALIDS /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
: 'RUN: at line 9';   cat /tmp/_update_lc/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.mod-out | /tmp/_update_lc/r/bin/FileCheck --check-prefix=CHECK-REALIDS /home/dave/ro_s/lp/clang/test/Modules/ModuleDebugInfoDwoId.cpp
--
Exit Code: 1


********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (1):
  Clang :: Modules/ModuleDebugInfoDwoId.cpp

I'm still not sure why the test is failing ("Exit Code: 1" isn't incredibly enlightening), so I reverted in 2b3074c0d14cadbd9595346fc795d4a49a479a20.

[davezarzycki]

Not fighting with how lit, FileCheck, and shell input/output work would help here:

FAIL: Clang :: Modules/ModuleDebugInfoDwoId.cpp (1 of 1)
******************** TEST 'Clang :: Modules/ModuleDebugInfoDwoId.cpp' FAILED ********************
Script:
--
: 'RUN: at line 3';   rm -rf /tmp/l/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache
: 'RUN: at line 4';   /tmp/l/r/bin/clang -cc1 -internal-isystem /tmp/l/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-unknown-linux-gnu -x objective-c++ -std=c++11 -debugger-tuning=lldb -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs -fimplicit-module-maps -fmodules-cache-path=/tmp/l/r/tools/clang/test/Modules/Output/ModuleDebugInfoDwoId.cpp.tmp.cache /home/dave/s/l/clang/test/Modules/ModuleDebugInfoDwoId.cpp -I /home/dave/s/l/clang/test/Modules/Inputs -emit-llvm -o - -mllvm -debug-only=pchcontainer | /tmp/l/r/bin/FileCheck --check-prefixes=CHECK,CHECK-REALIDS /home/dave/s/l/clang/test/Modules/ModuleDebugInfoDwoId.cpp
--
Exit Code: 2

Command Output (stderr):
--
clang (LLVM option parsing): Unknown command line argument '-debug-only=pchcontainer'.  Try: 'clang (LLVM option parsing) --help'
clang (LLVM option parsing): Did you mean '--debug-pass=pchcontainer'?
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /tmp/l/r/bin/FileCheck --check-prefixes=CHECK,CHECK-REALIDS /home/dave/s/l/clang/test/Modules/ModuleDebugInfoDwoId.cpp

--

********************
********************
Failed Tests (1):
  Clang :: Modules/ModuleDebugInfoDwoId.cpp

clang/include/clang/Basic/Module.h

struct ASTFileSignature : std::array<uint8_t, 20> {
   using BaseT = std::array<uint8_t, 20>;
 
   static constexpr size_t size = std::tuple_size<BaseT>::value;
 
   ASTFileSignature(BaseT S = {{0}}) : BaseT(std::move(S)) {}
 
   explicit operator bool() const { return *this != BaseT({{0}}); }
 
+  /// Returns the value truncated to the size of an uint64_t.
+  uint64_t truncatedValue() const {
+    uint64_t Value = 0;
+    static_assert(sizeof(*this) >= sizeof(uint64_t), "No need to truncate.");
+    for (unsigned I = 0; I < sizeof(uint64_t); ++I)
+      Value |= static_cast<uint64_t>((*this)[I]) << (I * 8);
+    return Value;
+  }
+
   static ASTFileSignature create(StringRef Bytes) {
     return create(Bytes.bytes_begin(), Bytes.bytes_end());
   }
 
   static ASTFileSignature createDISentinel() {
     ASTFileSignature Sentinel;
     Sentinel.fill(0xFF);
     return Sentinel;

clang/lib/CodeGen/CGDebugInfo.cpp

llvm::DIModule *CGDebugInfo::getOrCreateModuleRef(ASTSourceDescriptor Mod,
   };
 
   if (CreateSkeletonCU && IsRootModule && !Mod.getASTFile().empty()) {
     // PCH files don't have a signature field in the control block,
     // but LLVM detects skeleton CUs by looking for a non-zero DWO id.
     // We use the lower 64 bits for debug info.
 
     uint64_t Signature = 0;
-    if (const auto &ModSig = Mod.getSignature()) {
-      for (unsigned I = 0; I != sizeof(Signature); ++I)
-        Signature |= (uint64_t)ModSig[I] << (I * 8);
-    } else {
+    if (const auto &ModSig = Mod.getSignature())
+      Signature = ModSig.truncatedValue();
+    else
       Signature = ~1ULL;
-    }
+
     llvm::DIBuilder DIB(CGM.getModule());
     SmallString<0> PCM;
     if (!llvm::sys::path::is_absolute(Mod.getASTFile()))
       PCM = Mod.getPath();
     llvm::sys::path::append(PCM, Mod.getASTFile());
     DIB.createCompileUnit(
         TheCU->getSourceLanguage(),
         // TODO: Support "Source" from external AST providers?

clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp

if (Diags.hasErrorOccurred())
       return;
 
     M->setTargetTriple(Ctx.getTargetInfo().getTriple().getTriple());
     M->setDataLayout(Ctx.getTargetInfo().getDataLayout());
 
     // PCH files don't have a signature field in the control block,
     // but LLVM detects DWO CUs by looking for a non-zero DWO id.
     // We use the lower 64 bits for debug info.
+
     uint64_t Signature =
-        Buffer->Signature
-            ? (uint64_t)Buffer->Signature[1] << 32 | Buffer->Signature[0]
-            : ~1ULL;
+        Buffer->Signature ? Buffer->Signature.truncatedValue() : ~1ULL;
+
     Builder->getModuleDebugInfo()->setDwoId(Signature);
 
     // Finalize the Builder.
     if (Builder)
       Builder->Release();
 
     // Ensure the target exists.
     std::string Error;

clang/test/Modules/Inputs/DebugDwoId.h

+#ifndef DEBUG_DWO_ID_H
+#define DEBUG_DWO_ID_H
+struct Dummy {};
+#endif

clang/test/Modules/Inputs/module.map

 }
 
 module DebugObjCImport {
   module SubModule {
     header "DebugObjCImport.h"
   }
 }
 
+module DebugDwoId {
+  header "DebugDwoId.h"
+}
+
 module ImportNameInDir {
   header "ImportNameInDir.h"
   export *
 }
 
 module RequiresWithMissingHeader {
   module HeaderBefore {
     header "RequiresWithMissingHeader-Missing1.h"

clang/test/Modules/ModuleDebugInfoDwoId.cpp

+// Tests that dwoIds in modules match the dwoIDs in the main file.
+
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -triple %itanium_abi_triple -x objective-c++ -std=c++11 -debugger-tuning=lldb -debug-info-kind=limited -fmodules -fmodule-format=obj -dwarf-ext-refs -fimplicit-module-maps -fmodules-cache-path=%t.cache %s -I %S/Inputs -emit-llvm -o %t.ll -mllvm -debug-only=pchcontainer 2> %t.mod-out
+// RUN: cat %t.ll > %t.combined_output
+// RUN: cat %t.mod-out >> %t.combined_output
+// RUN: cat %t.combined_output | FileCheck %s
+// RUN: cat %t.ll | FileCheck --check-prefix=CHECK-REALIDS %s
+// RUN: cat %t.mod-out | FileCheck --check-prefix=CHECK-REALIDS %s
+
+@import DebugDwoId;
+
+Dummy d;
+
+// Find the emitted dwoID for DebugInfoId and compare it against the one in the PCM.
+// CHECK: DebugDwoId-{{[A-Z0-9]+}}.pcm
+// CHECK-SAME: dwoId: [[DWOID:[0-9]+]]
+// CHECK: dwoId: [[DWOID]]
+// CHECK-NEXT: !DIFile(filename: "DebugDwoId"
+
+// Make sure the dwo IDs are real IDs and not fallback values (~1ULL).
+// CHECK-REALIDS-NOT: dwoId: 18446744073709551615