[PATCH] D61926: Emit global variables as S_CONSTANT records for codeview debug info.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I was going to ask for a test case that makes local constants, but I see clang generates pretty weird metadata for this test case:

  int f(int x) {
    static const int MyConst = 42;
    return x + MyConst;
  }

That's supposed to generate a local S_CONSTANT, but we don't populate the DIExpression properly. Hm.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3000
   for (const CVGlobalVariable &CVGV : ComdatVariables) {
-    MCSymbol *GVSym = Asm->getSymbol(CVGV.GV);
+    const GlobalVariable *GV = CVGV.GVInfo.dyn_cast<const GlobalVariable *>();
+    MCSymbol *GVSym = Asm->getSymbol(GV);
----------------
I think it's impossible for S_CONSTANTs to be comdat because we find the comdat on the global variable, so I would suggest using `.get` instead of dyn_cast, so that we crash earlier if that invariant is violated.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3033-3034
+void CodeViewDebug::emitDebugInfoForGlobal(
+    const DIGlobalVariable *DIGV,
+    PointerUnion<const GlobalVariable *, const DIExpression *> GVInfo) {
+  if (const GlobalVariable *GV = GVInfo.dyn_cast<const GlobalVariable *>()) {
----------------
I think it would be simpler to have this take a `const CVGlobalVariable &` rather than taking the two members individually.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3055-3056
+    endSymbolRecord(DataEnd);
+  }
+  if (const DIExpression *DIE = GVInfo.dyn_cast<const DIExpression *>()) {
+    // FIXME: Currently this only emits the global variables in the IR metadata.
----------------
Maybe do this, so that this code complains loudly if another PointerUnion case is added instead of silently emitting nothing:
```
  } else {
    const DIExpression *DIE = GVInfo.get<const DIExpression *>();
```



================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3059
+    // This should also emit enums and static data members.
+    if (!DIE->isConstant())
+      return;
----------------
I think it would be better to check `isConstant()` earlier, before we set up the pointer union, so that no non-constant expressions make it here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61926