On Wed, Apr 29, 2015 at 9:58 AM Eric Christopher <[email protected]> wrote:
> On Wed, Apr 29, 2015 at 8:20 AM Adrian Prantl <[email protected]> wrote: > >> >> > On Apr 28, 2015, at 6:26 PM, David Blaikie <[email protected]> wrote: >> > >> > >> > >> > On Tue, Apr 28, 2015 at 4:01 PM, Adrian Prantl <[email protected]> >> wrote: >> > Author: adrian >> > Date: Tue Apr 28 18:01:24 2015 >> > New Revision: 236059 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=236059&view=rev >> > Log: >> > 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. >> > >> > 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. >> > >> > Seems we're not so lucky: >> http://lab.llvm.org:8011/builders/clang-x86_64-ubuntu-gdb-75/builds/21650/steps/gdb-75-check/logs/gdb.cp__anon-union.exp >> > >> > Could you revert & I'll look into getting you a reduced test >> case/demonstration of the issue? (can you run GDB 7.5? Perhaps a simple >> test case would demonstrate the issue if you're lucky, otherwise I can >> reduce one from the failing test case) >> >> I reverted the commit in r236110. I probably won’t need a reduction — my >> guess from the log is that gdb expects a local variable to be present. >> >> My suggestion is to emit local artificial shadow variables and then >> weaken the Verifier to not verify artificial variables. In a next step, we >> could use the new debugger tuning target feature to make the artificial >> local variables and the weakened verifier a gdb-specific behavior, file a >> bug against gdb, and eventually remove it altogether. >> >> > FWIW I don't want to use the "tuning" parameters to also affect > correctness. > OK. Dave and I debated this a little in person, here's a proposal: by default it will have the gdb specific behavior, but if you're tuning for lldb (or any other debugger I guess?) it won't be there. Thoughts? -eric > > -eric > > >> -- adrian >> >> > >> > >> > rdar://problem/20730771 >> > >> > Modified: >> > cfe/trunk/lib/CodeGen/CGDebugInfo.cpp >> > cfe/trunk/test/CodeGenCXX/debug-info-anon-union-vars.cpp >> > >> > Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=236059&r1=236058&r2=236059&view=diff >> > >> ============================================================================== >> > --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original) >> > +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Tue Apr 28 18:01:24 2015 >> > @@ -2835,31 +2835,6 @@ void CGDebugInfo::EmitDeclare(const VarD >> > 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. >> > >> > Modified: cfe/trunk/test/CodeGenCXX/debug-info-anon-union-vars.cpp >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-anon-union-vars.cpp?rev=236059&r1=236058&r2=236059&view=diff >> > >> ============================================================================== >> > --- cfe/trunk/test/CodeGenCXX/debug-info-anon-union-vars.cpp (original) >> > +++ cfe/trunk/test/CodeGenCXX/debug-info-anon-union-vars.cpp Tue Apr 28 >> 18:01:24 2015 >> > @@ -21,8 +21,26 @@ int test_it() { >> > 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]], >> > >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > [email protected] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
