r236059 - Debug Info: Represent local anonymous unions as anonymous unions

Eric Christopher echristo at gmail.com
Wed Apr 29 10:04:28 PDT 2015 

On Wed, Apr 29, 2015 at 9:58 AM Eric Christopher <echristo at gmail.com> wrote:

> On Wed, Apr 29, 2015 at 8:20 AM Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> > On Apr 28, 2015, at 6:26 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> >
>> > On Tue, Apr 28, 2015 at 4:01 PM, Adrian Prantl <aprantl at apple.com>
>> 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
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150429/654255ed/attachment.html>

More information about the cfe-commits mailing list