Page MenuHomePhabricator

[Debuginfo] Allow retrieving source files relative to the compilation directory.
ClosedPublic

Authored by saugustine on Jan 24 2020, 2:58 PM.

Details

Summary

Dwarf stores source-file names the three parts:
<compilation_directory><include_directory><filename>

Prior to this change, the code only allowed retrieving either all
three as the absolute path, or just the filename. But many
compile-command lines--especially those in hermetic build systems
don't specify an absolute path, nor just the filename, but rather the
path relative to the compilation directory. This features allows
retrieving them in that style.

Event Timeline

saugustine created this revision.Jan 24 2020, 2:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2020, 2:58 PM

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

This change looks like it's a little outside my area of understanding, so I've added a few of the DebugInfo guys as reviewers.

Where exactly is this new Kind actually created? At the moment, RelativeFilePath looks to be dead. Do you have a follow-up patch for it? Knowing where it is used would point towards the appropriate testing strategy, I think.

DWARF5 still says (when describing the directory entries): "Each additional path entry is either a full path name or is relative to the current directory of the compilation.", so I believe this change is correct.

However, I am puzzled by the addition of the RelativeFilePath enum, which is only used in an assertion.

Also, it seems like a test case would be in order here.

The enum is present only in an assert because as the code is written, it works as, an "other" case. It is something a client would pass, and right now there are no clients inside llvm. I guess I could add one to the symbolizer.

As I mentioned in the original message, I would love to add a test case for this, but I'm not sure where. The rest of the code isn't explicitly tested except by indirectly by way of symbolizer tests. So I guess I can go that route.

What flag would you like? Absolute is the symbolizer default, and "-s" makes it report basenames only.

The enum is present only in an assert because as the code is written, it works as, an "other" case. It is something a client would pass, and right now there are no clients inside llvm. I guess I could add one to the symbolizer.

Yeah, makes sense to me.

As I mentioned in the original message, I would love to add a test case for this, but I'm not sure where. The rest of the code isn't explicitly tested except by indirectly by way of symbolizer tests. So I guess I can go that route.

Right - LLVM has few API level unit tests (though there are some for libDebugInfoDWARF in llvm/unittests/DebugInfo/DWARF/ for instance), most testing is done by exposure through command line tools.

What flag would you like? Absolute is the symbolizer default, and "-s" makes it report basenames only.

Doesn't have to be super brief/punchy if we don't have a normal/common/regular use for it. I'd just go with "-no-comp-dir" for now.

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

That might be worthy of a separate patch & test case demonstrating the bug that's being fixed by moving that code out of the conditional.

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

That might be worthy of a separate patch & test case demonstrating the bug that's being fixed by moving that code out of the conditional.

https://reviews.llvm.org/D73583

The new test case fails without the accompanying fix.

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

That might be worthy of a separate patch & test case demonstrating the bug that's being fixed by moving that code out of the conditional.

https://reviews.llvm.org/D73583

The new test case fails without the accompanying fix.

Great, thanks! So once that's committed you can update this review with more test coverage in that test file?

labath resigned from this revision.Jan 29 2020, 12:13 AM

Also, it seems like a test case would be in order here.

My bad, I glossed over that part of the patch description. Anyway, I think others can do a better job at reviewing this.

This patch needs a rebase after D73583 is committed as 0758ac4e0cfbce67223ce2304e3b4b96c5006010.

Where exactly is this new Kind actually created? At the moment, RelativeFilePath looks to be dead. Do you have a follow-up patch for it? Knowing where it is used would point towards the appropriate testing strategy, I think.

Same question. Where is RelativeFilePath used? We can add a test to unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

saugustine updated this revision to Diff 243043.Feb 6 2020, 4:25 PM

Rebase for split out changes, and add a test.

jhenderson added inline comments.Feb 7 2020, 12:52 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1059

The first half of this check is dead: there's a return false earlier for Kind == FileLineInfoKind::None

I'd also like to see tests using None and Default kinds.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
909 ↗(On Diff #243043)

Rather than needing to create an entire custom prologue, you can use createBasicPrologue to generate a default one, and then just tweak the contents to suit your needs (not forgetting to update the length fields in the process). See ErrorForUnitLengthTooLarge for an example usage.

This will improve readability as it will more clearly show what is important to to this test.

949 ↗(On Diff #243043)

Could we use "compiler directory" in the comment please?

952 ↗(On Diff #243043)

No need to specify llvm:: here.

955 ↗(On Diff #243043)

I think you haven't clang-formatted this?

lebedev.ri retitled this revision from Allow retrieving source files relative to the compilation directory. to [Debuginfo] Allow retrieving source files relative to the compilation directory..Feb 7 2020, 12:59 AM
saugustine updated this revision to Diff 243300.Feb 7 2020, 3:23 PM
saugustine marked 5 inline comments as done.

Update for comments.

saugustine added inline comments.Feb 7 2020, 3:25 PM
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
909 ↗(On Diff #243043)

Doing this required modifying how basic prologues are created, which adds a few changes in random places. I had proposed D74249 separately, but the reviewer recommended I roll that into this.

saugustine edited the summary of this revision. (Show Details)Feb 7 2020, 3:26 PM

Aside from the inline comment, looks good to me.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
909 ↗(On Diff #243043)

As noted in D74249, you can avoid folding it in, by extending the checkDefaultPrologue checks.

dblaikie added inline comments.Feb 10 2020, 12:34 PM
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
909 ↗(On Diff #243043)

To avoid more churn in review, etc, I went ahead and committed D74249 with the suggested change to `checkDefaultPrologue``` ( in 0bd48c3d4ee1a94ea3d3b9d89201b23fd83c94d0 ) - so should be able to sync/resolve this patch with that and move forward from there.

Address upstream comments.

saugustine marked 2 inline comments as done.Feb 11 2020, 10:49 AM

I think this fixes everything. OK to submit?

dblaikie accepted this revision.Feb 11 2020, 11:11 AM

Sure - looks good :)

This revision is now accepted and ready to land.Feb 11 2020, 11:11 AM
This revision was automatically updated to reflect the committed changes.

From what I can tell, DILineInfoSpecifier::FileLineInfoKind::RelativeFilePath can only be set by unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp . Is there a plan to use it in other places of DebugInfo?