<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Dec 9, 2016 at 2:01 PM Adrian Prantl via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aprantl marked 8 inline comments as done.<br class="gmail_msg">
aprantl added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/IR/GlobalVariable.h:177<br class="gmail_msg">
+  using DIGlobalVarExpr =<br class="gmail_msg">
+      PointerUnion<DIGlobalVariable *, DIGlobalVariableExpression *>;<br class="gmail_msg">
+  /// Attach a DIGlobalVariable or DIGlobalVariableExpression.<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Is this union for backwards compatibility? (so things can still refer directly to a DIGlobalVariable?) If so, might be nice to push that compatibility earlier - map old debug info by creating new DIGlobalVariableExpressions, rather than letting the old semantics remain further through the system.<br class="gmail_msg">
The union (the canonical definition is in DebugInfoMetadata.h exists so we don't need to waste a DIGlobalVariableExpression node on global variables that don't need an DIExpression. This is an optimization that Duncan suggested (though I can't find where at the moment).<br class="gmail_msg"></blockquote><div><br></div><div>I assume the plan is to treat global variables the same way we treat functions - drop them if we no longer have a definition. (potentially keep a list of any that get collapsed to a constant/no asociation from an llvm::GlobalValue - but, say, if we did LTO and just optimized away a global variable because it was unused, we'd drop it the same as we drop functions). So it seems strange to me to optimize that case that I think we don't seem to be/intend to care about/have happen much/at all?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2209<br class="gmail_msg">
           }<br class="gmail_msg">
+          // FIXME: emitDebugInfoForGlobal() doesn't handle DIExpressions.<br class="gmail_msg">
           emitDebugInfoForGlobal(G, GV, Asm->getSymbol(GV));<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Not quite sure what this means/what the implications are?<br class="gmail_msg">
It's just something I noticed while updating the code. Could be a separate commit.<br class="gmail_msg"></blockquote><div><br>Might be good - just to discuss.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">================<br class="gmail_msg">
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:191<br class="gmail_msg">
+  if (Loc) {<br class="gmail_msg">
+    DwarfExpr->finalize();<br class="gmail_msg">
     addBlock(*VariableDIE, dwarf::DW_AT_location, Loc);<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Should finalize return the DIELoc instead of having it kept as a separate/parallel variable that DIEDwarfExpression is updating?<br class="gmail_msg">
Not sure I understand. You mean like<br class="gmail_msg">
```<br class="gmail_msg">
class DIEDwarfExpression {<br class="gmail_msg">
...<br class="gmail_msg">
  DIELoc finalize() {<br class="gmail_msg">
    DwarfExpression::finalize();<br class="gmail_msg">
    return DIE;<br class="gmail_msg">
  }<br class="gmail_msg">
}<br class="gmail_msg">
<br class="gmail_msg">
...<br class="gmail_msg">
addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr.finalize());<br class="gmail_msg"></blockquote><div><br>Yep, something like that. (presumably a DILoc*, but yeah - that idea) </div></div></div>