Page MenuHomePhabricator

CGDebugInfo: Drop Loc.isInvalid() special case from getLineNumber
Needs ReviewPublic

Authored by MaskRay on Mon, Jan 11, 12:55 AM.

Details

Reviewers
aprantl
dblaikie
rjmccall
Group Reviewers
debug-info
Summary

getLineNumber() picks CurLoc if the parameter is invalid. This appears to
mainly work around missing SourceLocation information for some constructs, but
sometimes adds unintended locations.

  • For CodeGenObjC/debug-info-blocks.m, CurLoc has been advanced to the closing brace. The debug line of ImplicitVarParameter is set to the line of } because this implicit parameter has an invalid SourceLocation. The debug line is a bit arbitrary - perhaps the location of ^{ is better.
  • (getOrCreateRecordFwdDecl) In clang/tools/driver/driver.cpp, the line of a #include is attached to DW_AT_decl_file/DW_AT_decl_line of a forward declaration __va_list_tag deep in the included hierarchy.

Drop the special case to make getLineNumber less magic and add CurLoc fallback in its callers instead.

Tested with stage 2 -DCMAKE_BUILD_TYPE=Debug clang, byte identical.

Diff Detail

Event Timeline

MaskRay requested review of this revision.Mon, Jan 11, 12:55 AM
MaskRay created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jan 11, 12:55 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

No particular bug. CGDebugInfo::getLineNumber returning CurLoc just looks strange. (The CurLoc may be far below (irrelevant) when getLineNumber is called.)

This patch is hopefully NFC. Dropping CurLoc from getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc) will cause a difference, which may reveal missing SourceLocation information in ObjC constructs.

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

No particular bug. CGDebugInfo::getLineNumber returning CurLoc just looks strange. (The CurLoc may be far below (irrelevant) when getLineNumber is called.)

This patch is hopefully NFC. Dropping CurLoc from getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc) will cause a difference, which may reveal missing SourceLocation information in ObjC constructs.

Ah, OK. Yeah, I don't have strong feelings about this - a bit hesitant to make the change without a stronger motivation though. Perhaps other reviewers have some thoughts to contribute (also maybe @JDevlieghere ).

MaskRay added a comment.EditedTue, Jan 12, 12:32 PM

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

No particular bug. CGDebugInfo::getLineNumber returning CurLoc just looks strange. (The CurLoc may be far below (irrelevant) when getLineNumber is called.)

This patch is hopefully NFC. Dropping CurLoc from getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc) will cause a difference, which may reveal missing SourceLocation information in ObjC constructs.

Ah, OK. Yeah, I don't have strong feelings about this - a bit hesitant to make the change without a stronger motivation though. Perhaps other reviewers have some thoughts to contribute (also maybe @JDevlieghere ).

Using CurLoc in getLineNumber is not ideal. Trying to fix one call site reveals a probably unsatisfactory debug location for ObjC ImplicitParamDecl.
The location is set to the closing brace. I do not investigate further as I know nearly nothing about ObjC...

--- a/clang/test/CodeGenObjC/debug-info-blocks.m
+++ b/clang/test/CodeGenObjC/debug-info-blocks.m
@@ -65,7 +65,7 @@ static void run(void (^block)(void))
       // CHECK-DAG: !DILocalVariable(arg: 2, scope: ![[COPY_SP]], {{.*}}, flags: DIFlagArtificial)
       // CHECK-DAG: !DILocalVariable(arg: 1, scope: ![[DESTROY_SP]], {{.*}}, flags: DIFlagArtificial)
       run(^{
-          // CHECK-DAG: ![[SELF]] = !DILocalVariable(name: "self", scope:{{.*}}, line: [[@LINE+4]],
+          // CHECK-DAG: ![[SELF]] = !DILocalVariable(name: "self", scope:{{.*}}, line: [[@LINE-7]],
           // CHECK-DAG: ![[D]] = !DILocalVariable(name: "d", scope:{{.*}}, line: [[@LINE+1]],
           NSMutableDictionary *d = [[NSMutableDictionary alloc] init]; 
           ivar = 42 + (int)[d count];

I think if the call sites are fixed, we can avoid future unintended introduction of undesired source locations.

MaskRay added a comment.EditedTue, Jan 12, 1:55 PM

I removed CurLoc from call sites and tried a stage 2 build. There is such a difference:

0x00062228:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("__va_list_tag")
                DW_AT_byte_size (0x18)
                DW_AT_decl_file ("/home/maskray/llvm/clang/tools/driver/driver.cpp")
                DW_AT_decl_line (23)

driver.cpp:23 is a #include. So this looks strange. The DW_AT_decl_file/DW_AT_decl_line attributes are undesired due to CurLoc getLineNumber.

I removed CurLoc from call sites and tried a stage 2 build. There is such a difference:

0x00062228:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("__va_list_tag")
                DW_AT_byte_size (0x18)
                DW_AT_decl_file ("/home/maskray/llvm/clang/tools/driver/driver.cpp")
                DW_AT_decl_line (23)

driver.cpp:23 is a #include. So this looks strange. The DW_AT_decl_file/DW_AT_decl_line attributes are undesired due to CurLoc getLineNumber.

I don't understand what you're describing - could you describe it in more detail/help me connect the dots?

It sounds like you're saying you removed the CurLoc fallback and then did a stage 2 build of clang and found an example of worse debug info (I'm not sure what file/line this struct was attributed to currently/without the change you were experimenting with) - that would suggest to me that the CurLoc fallback is helping, by providing a better location than the one you've mentioned here?

I like that this makes getLineNumber behave less "magic". I'm slightly worried that this isn't quite as NFC as think (and the other cases are just not caught by the testsuite). Perhaps you could compile Clang (and a sample of Clang's ObjC(xx) tests) with the patch and diff the dwarfdump output?

MaskRay updated this revision to Diff 316286.Tue, Jan 12, 5:37 PM
MaskRay edited the summary of this revision. (Show Details)

Add another workaround. stage 2 -DCMAKE_BUILD_TYPE=Debug clang is byte identical.

I removed CurLoc from call sites and tried a stage 2 build. There is such a difference:

0x00062228:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("__va_list_tag")
                DW_AT_byte_size (0x18)
                DW_AT_decl_file ("/home/maskray/llvm/clang/tools/driver/driver.cpp")
                DW_AT_decl_line (23)

driver.cpp:23 is a #include. So this looks strange. The DW_AT_decl_file/DW_AT_decl_line attributes are undesired due to CurLoc getLineNumber.

I don't understand what you're describing - could you describe it in more detail/help me connect the dots?

The ObjC example debug-info-blocks.m is: CurLoc has been advanced to the closing brace. Then getLineNumber is called on ImplicitVarParameter and the code attaches the location of } to this implicit parameter which misses SourceLocation. The location is a bit arbitrary - perhaps the location of ^{ is better.

A worse example is that CurLoc moves further away and getLineNumber is used to attach the line information to a far-away structure. This may potentially unintentionally apply to future AST nodes.

The DW_AT_decl_file __va_list_tag example is about attaching the line of a #include to some declaration deep in the included hierarchy.

It sounds like you're saying you removed the CurLoc fallback and then did a stage 2 build of clang and found an example of worse debug info (I'm not sure what file/line this struct was attributed to currently/without the change you were experimenting with) - that would suggest to me that the CurLoc fallback is helping, by providing a better location than the one you've mentioned here?

Clarified the description.

I'll leave this one to @aprantl

I'll leave this one to @aprantl

@aprantl :)