[PATCH] Debug Info: Represent local anonymous unions as anonymous unions.
Adrian Prantl
aprantl at apple.com
Tue Apr 28 16:05:45 PDT 2015
> On Apr 28, 2015, at 3:59 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Apr 28, 2015 at 3:52 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>> On Apr 28, 2015, at 3:50 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Tue, Apr 28, 2015 at 3:45 PM, Adrian Prantl <aprantl at apple.com> 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/
>>
>
>
More information about the cfe-commits
mailing list