[PATCH] D55336: [CodeView] Emit global variables within lexical scopes to limit visibility

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks, this looks like the right approach.



================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2750-2757
+  if (LI == ScopeVariables.end() && GI == ScopeGlobals.end()) {
     // This scope does not contain variables and can be eliminated.
-    collectLexicalBlockInfo(Scope.getChildren(), ParentBlocks, ParentLocals);
+    collectLexicalBlockInfo(Scope.getChildren(),
+                            ParentBlocks,
+                            ParentLocals,
+                            ParentGlobals);
     return;
----------------
I don't think this early case is needed now. Since you're checking against the end iterators again below, the condition might be simpler as `!Locals && !Globals`.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2795-2802
+    if (Locals)
+      ParentLocals.append(Locals->begin(), Locals->end());
+    if (Globals)
+      ParentGlobals.append(Globals->begin(), Globals->end());
+    collectLexicalBlockInfo(Scope.getChildren(),
+                            ParentBlocks,
+                            ParentLocals,
----------------
I see three cases here for scopes that we want to ignore. Can you simplify this code to compute a boolean, perhaps `IgnoreScope`, and then handle the early return case in a single `if (IgnoreScope)` block? I think all three cases can be handled uniformly, since you've already made the appending of variables conditional.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2976
+    MCSymbol *EndLabel = beginCVSubsection(DebugSubsectionKind::Symbols);
+    for (const CVGlobalVariable &CVGV : GlobalVariables) {
+      // FIXME: emitDebugInfoForGlobal() doesn't handle DIExpressions.
----------------
Can this become `emitGlobalVariableList(GlobalVariables)` now?


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3013
+  for (const CVGlobalVariable &CVGV : Globals) {
+    MCSymbol* GVSym = Asm->getSymbol(CVGV.GV);
+    emitDebugInfoForGlobal(CVGV.DIGV, CVGV.GV, GVSym);
----------------
For consistency, please match our "star on the right" coding style.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.h:191-195
   DenseMap<const LexicalScope *, SmallVector<LocalVariable, 1>> ScopeVariables;
 
+  // Map to seperate global variables according to the lexical scope they
+  // belong in. A null local scope represents the global scope.
+  DenseMap<const DIScope*, SmallVector<CVGlobalVariable, 1>> ScopeGlobals;
----------------
We really shouldn't make DenseMaps of SmallVectors, but that is a separate issue and I won't ask you to fix it.


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

https://reviews.llvm.org/D55336