Page MenuHomePhabricator

Correctly emit dwoIDs after ASTFileSignature refactoring (D81347)
ClosedPublic

Authored by teemperor on Jul 17 2020, 3:32 AM.

Details

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

Diff Detail

Event Timeline

teemperor created this revision.Jul 17 2020, 3:32 AM
dang added a comment.Jul 17 2020, 4:19 AM

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 updated this revision to Diff 278732.Jul 17 2020, 5:24 AM
  • Replaced min call with static_assert.
  • Fixed variable name.
teemperor marked 2 inline comments as done.Jul 17 2020, 5:26 AM

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 added inline comments.Jul 17 2020, 5:46 AM
clang/include/clang/Basic/Module.h
68

Nice!

dang accepted this revision.Jul 17 2020, 5:46 AM
This revision is now accepted and ready to land.Jul 17 2020, 5:46 AM
aprantl added inline comments.Jul 17 2020, 8:40 AM
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 added inline comments.Jul 18 2020, 8:54 AM
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 added inline comments.Aug 17 2020, 10:43 AM
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.

  • %t -> %t.cache
  • Don't concat out files and instead run FileCheck on each file where possible.
aprantl accepted this revision.Aug 19 2020, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2020, 6:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
teemperor reopened this revision.Aug 21 2020, 7:09 AM

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.

This revision is now accepted and ready to land.Aug 21 2020, 7:09 AM

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

teemperor reopened this revision.Aug 24 2020, 4:46 AM
This revision is now accepted and ready to land.Aug 24 2020, 4:46 AM

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