Page MenuHomePhabricator

[Debug][CodeView] Emit fully qualified names for globals
ClosedPublic

Authored by aganea on May 5 2020, 1:48 PM.

Details

Summary

Before this patch, S_[L|G][THREAD32|DATA32] records were emitted with a simple name, not the fully qualified name (namespace + class scope). I've simply reused what @akhuang did for S_CONSTANT

Diff Detail

Event Timeline

aganea created this revision.May 5 2020, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2020, 1:48 PM
rnk accepted this revision.May 5 2020, 3:35 PM

lgtm

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
3134–3135

@akhuang, can we remove this FIXME? I think we addressed this simply by changing clang to emit this metadata.

This revision is now accepted and ready to land.May 5 2020, 3:35 PM
akhuang added inline comments.May 5 2020, 3:46 PM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
3134–3135

Yep- thanks for catching that!

aganea marked an inline comment as done.EditedWed, May 6, 5:39 AM

Thanks!

I have a backlog of issues I'm noting along the way, I will only fix them if they have an impact in regards to debugging in Visual Studio.

  1. The list of namespaces, ie.S_NAMESPACE isn't emitted in the first module.
  2. extern int A; doesn't emit anything in the debug stream (of a .OBJ). MSVC emits S_GDATA32, but maybe this doesn't matter as it will be emitted probably by the TU that defines it.
  3. S_OBJNAME is not emitted (see D43002, I will get back to this soon).
  4. LF_BUILDINFO does not contain the full repro for rebuilding the .OBJ. I will fix this soon as well, this is blocking us.
  5. LF_NESTTYPE emits empty names for inner types (at least not a unnamed enum).
  6. Negative enumerations are emitted as U64, not signed. I need to check if this has an impact in the VS debugger.
  7. S_UDT isn't emitted for a global enum.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
3134–3135

Can I leave the comment for 'enum'? There are still a few things missing in the debug stream, in regards to enums (see other comment).

This revision was automatically updated to reflect the committed changes.
aganea updated this revision to Diff 263450.Tue, May 12, 8:45 AM

Reopening because I had to revert.

  • Fix lldb's variables.test
  • Fix type lowering, as described here: D79512
  • Added assert in emitDebugInfoForUDTs to ensure no lowering can occur during UDT emission, as suggested by @rnk
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 12, 8:45 AM
aganea reopened this revision.Tue, May 12, 8:46 AM

Could you please take another look? Thank you in advance!

This revision is now accepted and ready to land.Tue, May 12, 8:46 AM
aganea requested review of this revision.Tue, May 12, 8:46 AM
rnk accepted this revision.Thu, May 14, 12:10 PM

lgtm, modulo ifdef adjustment

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2989

This probably needs to be #ifndef NDEBUG, or a build with asserts but without ABI breaking checks (blech) will break. Besides, it's not an ABI breaking check.

3134–3135

I'd leave the FIXME out, as you have it now. I don't think we're getting value from it here. At this point, it's in the frontend's hands to decide if the enum needs emitting, so the code fix would be far from this comment.

This revision is now accepted and ready to land.Thu, May 14, 12:10 PM
This revision was automatically updated to reflect the committed changes.
aganea marked 4 inline comments as done.
hans added a subscriber: hans.Mon, May 18, 2:28 AM
  • Added assert in emitDebugInfoForUDTs to ensure no lowering can occur during UDT emission, as suggested by @rnk

It seems we hit the assert in Chromium. Here's a reproducer: https://bugs.chromium.org/p/chromium/issues/detail?id=1083877#c3 (I'll try to create a shorter one.)

I've reverted in 525a591f0f48b9d54018bf5245f2abee09c9c1c8 in the meantime.

hans added a comment.Mon, May 18, 5:33 AM
  • Added assert in emitDebugInfoForUDTs to ensure no lowering can occur during UDT emission, as suggested by @rnk

It seems we hit the assert in Chromium. Here's a reproducer: https://bugs.chromium.org/p/chromium/issues/detail?id=1083877#c3 (I'll try to create a shorter one.)

I've reverted in 525a591f0f48b9d54018bf5245f2abee09c9c1c8 in the meantime.

Reduced repro:

$ cat /tmp/x.ii
typedef int a;
struct b;
class c {
  c();
  a b::*d;
};
c::c() = default;

$ clang -cc1 -triple x86_64-pc-windows-msvc19.16.0 -emit-obj -gcodeview -debug-info-kind=limited /tmp/x.ii
clang: /work/llvm.monorepo/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2997: void llvm::CodeViewDebug::emitDebugInfoForUDTs(const std::vector<std::pair<std::string, const DIType *>> &): Assertion `OriginalSize == UDTs.size()' failed.
rnk added a comment.Mon, May 18, 12:08 PM

I will take a look and try to reland this, since I requested the assert.