[PATCH] D26769: [IR] Remove the DIExpression field from DIGlobalVariable.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

It'd probably be nice for the upgraded tests to not use the inline metadata representation, but have another numbered node (just for consistency - because we don't have any DI metadata like that now, and I doubt we'll write it that way by hand in the future either, so it'll be out of place, etc (possibly making future auto upgrades more difficult by having more varied/odd/surprising representations to match/handle)). But I understand if that might be difficult to find a valid number for the newly introduced node.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:662
+    EntityDie = getOrCreateGlobalVariableDIE(GV, {});
+  else if (auto *GVE = dyn_cast<DIGlobalVariableExpression>(Entity)) {
+    GlobalExpr GE = {nullptr, GVE->getExpression()};
----------------
This seems strange - when/how would an imported entity refer to a DIGlobalVariableExpression? It should only ever refer to a DIGlobalVariable, right?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:498-499
+    // Global Variables.
+    for (auto *GVE : CUNode->getGlobalVariables())
+      GVMap[GVE->getVariable()].push_back({nullptr, GVE->getExpression()});
+    DenseSet<DIGlobalVariable *> Processed;
----------------
Wouldn't this leave the GVs from other CUs in the GVMap, causing them to be emitted into future CUs as well?

I guess the Processed check below avoids that? Could we avoid it by design in some other way, perhaps? For example when we go to insert something into the GVMap, if it's a new insertion we could emit it, otherwise we could ignore it? (I suppose this wouldn't work for the things that are put in the GVMap by the globals() loop above?)

Does the CU's global variable list currently contain all the globals (not just the constants?)? Then what's the need for the globals() loop above? To discover all instances of a global variable across CUs because they might all contribute to the location of the global in debug info?


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1656
     G->getDebugInfo(GVs);
-    for (auto *GV : GVs)
+    for (auto GV : GVs)
       NewGlobal->addDebugInfo(GV);
----------------
Might be worth putting the type here instead of auto so it's clear it's a handle type that can be copied intentionally/cheaply/etc.


https://reviews.llvm.org/D26769