> On Apr 28, 2015, at 3:59 PM, David Blaikie <[email protected]> wrote: > > > > On Tue, Apr 28, 2015 at 3:52 PM, Adrian Prantl <[email protected]> wrote: > >> On Apr 28, 2015, at 3:50 PM, David Blaikie <[email protected]> wrote: >> >> >> >> On Tue, Apr 28, 2015 at 3:45 PM, Adrian Prantl <[email protected]> wrote: >> Hi echristo, dblaikie, >> >> Debug Info: Represent local anonymous unions as anonymous unions in the >> debug info. >> This patch deletes a hack that emits the members of local anonymous unions >> as local variables. >> >> Why? Besides being morally wrong, the existing representation using local >> variables breaks internal assumptions about the local variables' storage >> size. >> >> Compiling >> >> ``` >> void fn1() { >> union { >> int i; >> char c; >> }; >> i = c; >> } >> >> ``` >> >> 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. >> >> 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. >> >> Eric, do you know whether the local variable trick is necessary for GDB >> compatibility? >> >> 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). >> >> GCC produces something vaguely interesting: >> >> 0x0000002d: DW_TAG_subprogram [2] * >> ... >> 0x0000004e: DW_TAG_lexical_block [3] * >> ... >> >> 0x00000063: DW_TAG_variable [4] >> DW_AT_name [DW_FORM_string] ("i") >> DW_AT_type [DW_FORM_ref4] (cu + 0x0098 => {0x00000098}) >> DW_AT_artificial [DW_FORM_flag_present] (true) >> DW_AT_location [DW_FORM_exprloc] (<0x2> 91 60 ) >> >> 0x0000006d: DW_TAG_variable [4] >> DW_AT_name [DW_FORM_string] ("c") >> DW_AT_type [DW_FORM_ref4] (cu + 0x009f => {0x0000009f}) >> DW_AT_artificial [DW_FORM_flag_present] (true) >> DW_AT_location [DW_FORM_exprloc] (<0x2> 91 60 ) >> >> 0x00000077: DW_TAG_variable [5] >> DW_AT_type [DW_FORM_ref4] (cu + 0x0080 => {0x00000080}) >> DW_AT_location [DW_FORM_exprloc] (<0x2> 91 60 ) >> >> 0x0000007f: NULL >> >> 0x00000080: DW_TAG_union_type [6] * >> DW_AT_byte_size [DW_FORM_data1] (0x04) >> DW_AT_decl_file [DW_FORM_data1] >> ("/tmp/dbginfo/union.c") >> DW_AT_decl_line [DW_FORM_data1] (2) >> >> 0x00000084: DW_TAG_member [7] >> DW_AT_name [DW_FORM_string] ("i") >> DW_AT_decl_file [DW_FORM_data1] >> ("/tmp/dbginfo/union.c") >> DW_AT_decl_line [DW_FORM_data1] (3) >> DW_AT_type [DW_FORM_ref4] (cu + 0x0098 => {0x00000098}) >> >> 0x0000008d: DW_TAG_member [7] >> DW_AT_name [DW_FORM_string] ("c") >> DW_AT_decl_file [DW_FORM_data1] >> ("/tmp/dbginfo/union.c") >> DW_AT_decl_line [DW_FORM_data1] (4) >> DW_AT_type [DW_FORM_ref4] (cu + 0x009f => {0x0000009f}) >> >> 0x00000096: NULL >> >> 0x00000097: NULL > > 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. > > 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. > > 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.
Thanks! Committed as r236059. In the worst case I could compromise by implementing the shadow variables and have the verifier ignore artificial variables, but let’s see if that’s necessary. -- adrian > > - David > > > -- adrian > >> >> >> >> >> REPOSITORY >> rL LLVM >> >> http://reviews.llvm.org/D9329 >> >> Files: >> lib/CodeGen/CGDebugInfo.cpp >> test/CodeGenCXX/debug-info-anon-union-vars.cpp >> >> Index: lib/CodeGen/CGDebugInfo.cpp >> =================================================================== >> --- lib/CodeGen/CGDebugInfo.cpp >> +++ lib/CodeGen/CGDebugInfo.cpp >> @@ -2835,31 +2835,6 @@ >> return; >> } else if (isa<VariableArrayType>(VD->getType())) >> Expr.push_back(llvm::dwarf::DW_OP_deref); >> - } else if (const RecordType *RT = dyn_cast<RecordType>(VD->getType())) { >> - // If VD is an anonymous union then Storage represents value for >> - // all union fields. >> - const RecordDecl *RD = cast<RecordDecl>(RT->getDecl()); >> - if (RD->isUnion() && RD->isAnonymousStructOrUnion()) { >> - for (const auto *Field : RD->fields()) { >> - llvm::MDType *FieldTy = getOrCreateType(Field->getType(), Unit); >> - StringRef FieldName = Field->getName(); >> - >> - // Ignore unnamed fields. Do not ignore unnamed records. >> - if (FieldName.empty() && !isa<RecordType>(Field->getType())) >> - continue; >> - >> - // Use VarDecl's Tag, Scope and Line number. >> - auto *D = DBuilder.createLocalVariable( >> - Tag, Scope, FieldName, Unit, Line, FieldTy, >> - CGM.getLangOpts().Optimize, Flags, ArgNo); >> - >> - // Insert an llvm.dbg.declare into the current block. >> - DBuilder.insertDeclare(Storage, D, DBuilder.createExpression(Expr), >> - llvm::DebugLoc::get(Line, Column, Scope), >> - Builder.GetInsertBlock()); >> - } >> - return; >> - } >> } >> >> // Create the descriptor for the variable. >> Index: test/CodeGenCXX/debug-info-anon-union-vars.cpp >> =================================================================== >> --- test/CodeGenCXX/debug-info-anon-union-vars.cpp >> +++ test/CodeGenCXX/debug-info-anon-union-vars.cpp >> @@ -21,8 +21,26 @@ >> return (c == 1); >> } >> >> +// This is not necessary (and actually harmful because it breaks IR >> assumptions) >> +// for local variables. >> +void foo() { >> + union { >> + int i; >> + char c; >> + }; >> + i = 8; >> +} >> + >> // CHECK: [[FILE:.*]] = !MDFile(filename: >> "{{.*}}debug-info-anon-union-vars.cpp", >> // CHECK: !MDGlobalVariable(name: "c",{{.*}} file: [[FILE]], line: 6,{{.*}} >> isLocal: true, isDefinition: true >> // CHECK: !MDGlobalVariable(name: "d",{{.*}} file: [[FILE]], line: 6,{{.*}} >> isLocal: true, isDefinition: true >> // CHECK: !MDGlobalVariable(name: "a",{{.*}} file: [[FILE]], line: 6,{{.*}} >> isLocal: true, isDefinition: true >> // CHECK: !MDGlobalVariable(name: "b",{{.*}} file: [[FILE]], line: 6,{{.*}} >> isLocal: true, isDefinition: true >> +// CHECK: !MDLocalVariable( >> +// CHECK-NOT: name: >> +// CHECK: type: ![[UNION:[0-9]+]] >> +// CHECK: ![[UNION]] = !MDCompositeType(tag: DW_TAG_union_type, >> +// CHECK-NOT: name: >> +// CHECK: elements >> +// CHECK: !MDDerivedType(tag: DW_TAG_member, name: "i", scope: ![[UNION]], >> +// CHECK: !MDDerivedType(tag: DW_TAG_member, name: "c", scope: ![[UNION]], >> >> EMAIL PREFERENCES >> http://reviews.llvm.org/settings/panel/emailpreferences/ >> > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
