Page MenuHomePhabricator

[DebugInfo] Support file-level include directories when generating Dwarf5 LineTable prologues.
ClosedPublic

Authored by saugustine on Feb 7 2020, 12:25 PM.

Event Timeline

saugustine created this revision.Feb 7 2020, 12:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2020, 12:25 PM

It'd be helpful to add something that exercises the new functionality along with the functionality (not necessarily a test /for/ the functionality (since this is test functionality, and then you get in the infinite rabbit hole of testing testing) - but potentially in some libDEbugInfo unit test that uses this functionality and wouldn't be possible without it?)

It'd be helpful to add something that exercises the new functionality along with the functionality (not necessarily a test /for/ the functionality (since this is test functionality, and then you get in the infinite rabbit hole of testing testing) - but potentially in some libDEbugInfo unit test that uses this functionality and wouldn't be possible without it?)

D73383 will use this functionality, but because it is logically separate, I thought it best to submit it separately.

It'd be helpful to add something that exercises the new functionality along with the functionality (not necessarily a test /for/ the functionality (since this is test functionality, and then you get in the infinite rabbit hole of testing testing) - but potentially in some libDEbugInfo unit test that uses this functionality and wouldn't be possible without it?)

D73383 will use this functionality, but because it is logically separate, I thought it best to submit it separately.

Yeah, it's a tricky tradeoff - trying to isolate changes, but equally trying not to commit untested/unverified functionality.

If it's possible to/you could refactor the constants (in DWARFDebugLineTest.cpp and dwarfgen::LineTable::createBasicPrologue ) so they're symbolic/don't need to be updated when writeV5IncludeAndFileTable is made - taht'd be a good isolated "NFC" commit. But no worries if that isn't possible.

Otherwise/please roll this into the other review, so the functionality is exercised when it's committed.

The change itself mostly looks fine to me, FWIW. An alternative to folding it into the other review would be to make a change in checkDefaultPrologue to check the new property of the FileNames, just like the name is already checked. The existing tests that use this function would then validate your changes.

I haven't been particularly happy with the constants in the DwarfGenerator code, although I have yet to come up with a viable alternative (not that I've thought about it all that much).

The change itself mostly looks fine to me, FWIW. An alternative to folding it into the other review would be to make a change in checkDefaultPrologue to check the new property of the FileNames, just like the name is already checked. The existing tests that use this function would then validate your changes.

Ah, makes sense. To avoid going through more cycles of review I went ahead and committed this patch with that suggested change in 0bd48c3d4ee1a94ea3d3b9d89201b23fd83c94d0

This revision was not accepted when it landed; it landed in state Needs Review.Feb 10 2020, 12:37 PM
This revision was automatically updated to reflect the committed changes.