[PATCH] D79028: [DebugInfo][CodeView] Include namespace into emitted globals

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a subscriber: akhuang.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3146
     emitNullTerminatedSymbolName(OS,
                                  getFullyQualifiedName(Scope, DIGV->getName()));
     endSymbolRecord(SConstantEnd);
----------------
Looks like @akhuang got this right recently for S_CONSTANT, but we've had the bug with global names since forever. :(


================
Comment at: llvm/test/DebugInfo/COFF/globals.ll:145
-; YAML:        SymbolName:      '?first@@3HA'
-; YAML:        Type:            IMAGE_REL_AMD64_SECREL
-; YAML:      - VirtualAddress:  96
----------------
I'm not sure why we check the relocations here in the first place. They use offsets, so they tend to be fragile. I think we ought to remove them from this test. We have enough coverage from the ASM check lines above. As long as we have confidence that .secrel and .secidx directives work and are tested elsewhere, this seems redundant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79028/new/

https://reviews.llvm.org/D79028