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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


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

```
namespace { // Force c to be local.
  namespace A {
    int c = 42;
  }
}
using A::c;
```
Unfortunately it doesn't look like this works at the moment. We're dropping the constant somewhere.


================
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;
----------------
dblaikie wrote:
> 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?
GVMap is used to collect all `DIGlobalVariableExpression`s that contribute to on global variable's location in the debug info. We need to iterate over all globals once to establish this mapping.

Then, for each global variable in the current CU we emit debug info for it and insert it into Processed to make sure we don't do it again.

I don't think that this two-step process can be shortened; let me know if you see a way that I missed.
But, I think that there is still an unhandled case, and that is a global variable with two constants in DIExpressions and no global symbol. I'll fix that and add a testcase.

Finally, to your last question: Yes, they are listed in the CU, but the global symbol is not reachable from the metadata; we refer to metadata via the `!dbg` attachment, not the other way round.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1656
     G->getDebugInfo(GVs);
-    for (auto *GV : GVs)
+    for (auto GV : GVs)
       NewGlobal->addDebugInfo(GV);
----------------
dblaikie wrote:
> 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.
I think this should just stay an `auto *`.  This is a leftover from the PointerUnion days.


https://reviews.llvm.org/D26769