[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:423 + if (!CSInfo) { +SmallString<64> Checksum; +std::optional CSKind = In the final commit, `Checksum` is outside the `if` so that its lifetime persists to the end of the fu

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. Thanks @DavidSpickett the patch is currently reverted. I have a revised patch coming soon and I will keep a close eye on the bots. I believe it's a string-lifetime issue and so whether it manifests is unpredictable, but we have enough different bots in the farm that it

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

DavidSpickett added a comment. Failing on one of our 2 stage AArch64 bots https://lab.llvm.org/buildbot/#/builders/176/builds/4267 after the reland (that bot has been red for various reasons today, so no emails got sent). Maybe it helps to know that it doesn't fail on the same config with singl

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. This patch is possibly a suspect in at least some bot failures although I'm at a loss to understand why. Perhaps I can't just blithely call getChecksum() and copy what it sends back? The ways of metadata remai

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

nikic added a comment. Thanks! Can confirm that this recovers the compile-time regression. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156571/new/ https://reviews.llvm.org/D156571 ___ cfe-commits maili

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. Detail added in the commit message, good idea! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156571/new/ https://reviews.llvm.org/D156571 ___ cfe-commits mailing list cfe-commi

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGca1295c5a15f: [DebugInfo] Alternate (more efficient) MD5 fix (authored by probinson). Herald added a project: clang. Repository: rG LLVM Github Mo

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds OK to me - thanks! bit more detail in the actual patch description/commit message (from some of the comments in this review, perhaps) would be handy. CHANGES SINCE LAST ACTION h

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156571/new/ https://reviews.llvm.org/D156571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson updated this revision to Diff 546144. probinson added a comment. Reuse the main file's checksum instead of recalculating it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156571/new/ https://reviews.llvm.org/D156571 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeG

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. Of course this means we're rerunning MD5 on the main source file; probably can capture that from TheCU and save that cost as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156571/new/ https://reviews.llvm.org/D156571

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson updated this revision to Diff 546115. probinson added a comment. Use the main FileID instead of expensive string compares. Figured this out after staring at CreateCompileUnit for long enough. Seeding the DIFileCache with the DIFile created there made another test unhappy (difile_entry

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. The CU's DIFile is conjured up in CGDebugInfo::CreateCompileUnit(), and the name is derived from `-main-file-name` rather than anything SourceManager provides. Although there is a comment wondering about that. CreateCompileUnit() computes a checksum for that DIFile, b

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

dblaikie added a comment. From the other review: >> Could/should we do the lookup on the CU filename before it goes into the DI >> metadata, and store that FileID somewhere for later use? > > There's a note in issue 63955 to the effect that I can't find an API to turn > a filename into a FileID

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson created this revision. probinson added reviewers: dblaikie, nikic. probinson added a project: debug-info. Herald added a subscriber: StephenFan. Herald added a project: All. probinson requested review of this revision. Should have lower memory and time cost than D155991