<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 28, 2015 at 3:52 PM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div class="h5"><blockquote type="cite"><div>On Apr 28, 2015, at 3:50 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 28, 2015 at 3:45 PM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi echristo, dblaikie,<br>
<br>
Debug Info: Represent local anonymous unions as anonymous unions in the debug info.<br>
This patch deletes a hack that emits the members of local anonymous unions as local variables.<br>
<br>
Why? Besides being morally wrong, the existing representation using local variables breaks internal assumptions about the local variables' storage size.<br>
<br>
Compiling<br>
<br>
```<br>
    void fn1() {<br>
      union {<br>
        int i;<br>
        char c;<br>
      };<br>
      i = c;<br>
    }<br>
<br>
```<br>
<br>
with -g -O3 -verify will cause the verifier to fail after SROA splits the 32-bit storage for the "local variable" c into two pieces because the second piece is clearly outside the 8-bit range that is expected for a variable of type char. Given the choice I'd rather fix the debug representation than weaken the verifier.<br>
<br>
Debuggers generally already know how to deal with anonymous unions when they are members of C++ record types, but they may have problems finding the local anonymous struct members in the expression evaluator.<br>
<br>
Eric, do you know whether the local variable trick is necessary for GDB compatibility?<br></blockquote><div><br>Throwing it at the GDB buildbot's probably the best way to find out. Don't too much mind committing it first & seeing if it fails (unless you guys can run it these days, in which case a preliminary run would be nice).<br><br>GCC produces something vaguely interesting:<br><br><div><font face="monospace, monospace">0x0000002d:   DW_TAG_subprogram [2] *</font></div><div><font face="monospace">                  ...</font></div><div><font face="monospace, monospace">0x0000004e:     DW_TAG_lexical_block [3] *<br>                  ...</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x00000063:       DW_TAG_variable [4]  </font></div><div><font face="monospace, monospace">                    DW_AT_name [DW_FORM_string] ("i")</font></div><div><font face="monospace, monospace">                    DW_AT_type [DW_FORM_ref4]   (cu + 0x0098 => {0x00000098})</font></div><div><font face="monospace, monospace">                    DW_AT_artificial [DW_FORM_flag_present]     (true)</font></div><div><font face="monospace, monospace">                    DW_AT_location [DW_FORM_exprloc]    (<0x2> 91 60 )</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x0000006d:       DW_TAG_variable [4]  </font></div><div><font face="monospace, monospace">                    DW_AT_name [DW_FORM_string] ("c")</font></div><div><font face="monospace, monospace">                    DW_AT_type [DW_FORM_ref4]   (cu + 0x009f => {0x0000009f})</font></div><div><font face="monospace, monospace">                    DW_AT_artificial [DW_FORM_flag_present]     (true)</font></div><div><font face="monospace, monospace">                    DW_AT_location [DW_FORM_exprloc]    (<0x2> 91 60 )</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x00000077:       DW_TAG_variable [5]  </font></div><div><font face="monospace, monospace">                    DW_AT_type [DW_FORM_ref4]   (cu + 0x0080 => {0x00000080})</font></div><div><font face="monospace, monospace">                    DW_AT_location [DW_FORM_exprloc]    (<0x2> 91 60 )</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x0000007f:       NULL</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x00000080:     DW_TAG_union_type [6] *</font></div><div><font face="monospace, monospace">                  DW_AT_byte_size [DW_FORM_data1]       (0x04)</font></div><div><font face="monospace, monospace">                  DW_AT_decl_file [DW_FORM_data1]       ("/tmp/dbginfo/union.c")</font></div><div><font face="monospace, monospace">                  DW_AT_decl_line [DW_FORM_data1]       (2)</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x00000084:       DW_TAG_member [7]  </font></div><div><font face="monospace, monospace">                    DW_AT_name [DW_FORM_string] ("i")</font></div><div><font face="monospace, monospace">                    DW_AT_decl_file [DW_FORM_data1]     ("/tmp/dbginfo/union.c")</font></div><div><font face="monospace, monospace">                    DW_AT_decl_line [DW_FORM_data1]     (3)</font></div><div><font face="monospace, monospace">                    DW_AT_type [DW_FORM_ref4]   (cu + 0x0098 => {0x00000098})</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x0000008d:       DW_TAG_member [7]  </font></div><div><font face="monospace, monospace">                    DW_AT_name [DW_FORM_string] ("c")</font></div><div><font face="monospace, monospace">                    DW_AT_decl_file [DW_FORM_data1]     ("/tmp/dbginfo/union.c")</font></div><div><font face="monospace, monospace">                    DW_AT_decl_line [DW_FORM_data1]     (4)</font></div><div><font face="monospace, monospace">                    DW_AT_type [DW_FORM_ref4]   (cu + 0x009f => {0x0000009f})</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x00000096:       NULL</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">0x00000097:     NULL</font></div></div></div></div></div></div></blockquote><div><br></div></div></div>That’s pretty much the output that this patch produces, too (anonymous local variable + anonymous union). If we can use GCC as a precedent, I’m happy to commit this and see whether it breaks.</div></div></blockquote><div><br>The interesting thing in GCC's output is that it produced 3 variables in the function - one is the union, the other two are named variables that are marked artificial. I hope that's not necessary, but don't rightly know.<br><br>The code change + test seem good - feel free to commit & keep an eye on the GDB buildbot. If it fails for good reasons it's probably best to revert & I'll try to get you a reduction.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><span class="HOEnZb"><font color="#888888"><div><br></div></font></span><div><span class="HOEnZb"><font color="#888888">-- adrian</font></span><div><div class="h5"><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
REPOSITORY<br>
  rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D9329" target="_blank">http://reviews.llvm.org/D9329</a><br>
<br>
Files:<br>
  lib/CodeGen/CGDebugInfo.cpp<br>
  test/CodeGenCXX/debug-info-anon-union-vars.cpp<br>
<br>
Index: lib/CodeGen/CGDebugInfo.cpp<br>
===================================================================<br>
--- lib/CodeGen/CGDebugInfo.cpp<br>
+++ lib/CodeGen/CGDebugInfo.cpp<br>
@@ -2835,31 +2835,6 @@<br>
       return;<br>
     } else if (isa<VariableArrayType>(VD->getType()))<br>
       Expr.push_back(llvm::dwarf::DW_OP_deref);<br>
-  } else if (const RecordType *RT = dyn_cast<RecordType>(VD->getType())) {<br>
-    // If VD is an anonymous union then Storage represents value for<br>
-    // all union fields.<br>
-    const RecordDecl *RD = cast<RecordDecl>(RT->getDecl());<br>
-    if (RD->isUnion() && RD->isAnonymousStructOrUnion()) {<br>
-      for (const auto *Field : RD->fields()) {<br>
-        llvm::MDType *FieldTy = getOrCreateType(Field->getType(), Unit);<br>
-        StringRef FieldName = Field->getName();<br>
-<br>
-        // Ignore unnamed fields. Do not ignore unnamed records.<br>
-        if (FieldName.empty() && !isa<RecordType>(Field->getType()))<br>
-          continue;<br>
-<br>
-        // Use VarDecl's Tag, Scope and Line number.<br>
-        auto *D = DBuilder.createLocalVariable(<br>
-            Tag, Scope, FieldName, Unit, Line, FieldTy,<br>
-            CGM.getLangOpts().Optimize, Flags, ArgNo);<br>
-<br>
-        // Insert an llvm.dbg.declare into the current block.<br>
-        DBuilder.insertDeclare(Storage, D, DBuilder.createExpression(Expr),<br>
-                               llvm::DebugLoc::get(Line, Column, Scope),<br>
-                               Builder.GetInsertBlock());<br>
-      }<br>
-      return;<br>
-    }<br>
   }<br>
<br>
   // Create the descriptor for the variable.<br>
Index: test/CodeGenCXX/debug-info-anon-union-vars.cpp<br>
===================================================================<br>
--- test/CodeGenCXX/debug-info-anon-union-vars.cpp<br>
+++ test/CodeGenCXX/debug-info-anon-union-vars.cpp<br>
@@ -21,8 +21,26 @@<br>
   return (c == 1);<br>
 }<br>
<br>
+// This is not necessary (and actually harmful because it breaks IR assumptions)<br>
+// for local variables.<br>
+void foo() {<br>
+  union {<br>
+    int i;<br>
+    char c;<br>
+  };<br>
+  i = 8;<br>
+}<br>
+<br>
 // CHECK: [[FILE:.*]] = !MDFile(filename: "{{.*}}debug-info-anon-union-vars.cpp",<br>
 // CHECK: !MDGlobalVariable(name: "c",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true<br>
 // CHECK: !MDGlobalVariable(name: "d",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true<br>
 // CHECK: !MDGlobalVariable(name: "a",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true<br>
 // CHECK: !MDGlobalVariable(name: "b",{{.*}} file: [[FILE]], line: 6,{{.*}} isLocal: true, isDefinition: true<br>
+// CHECK: !MDLocalVariable(<br>
+// CHECK-NOT: name:<br>
+// CHECK: type: ![[UNION:[0-9]+]]<br>
+// CHECK: ![[UNION]] = !MDCompositeType(tag: DW_TAG_union_type,<br>
+// CHECK-NOT: name:<br>
+// CHECK: elements<br>
+// CHECK: !MDDerivedType(tag: DW_TAG_member, name: "i", scope: ![[UNION]],<br>
+// CHECK: !MDDerivedType(tag: DW_TAG_member, name: "c", scope: ![[UNION]],<br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
</blockquote></div><br></div></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>