This is an archive of the discontinued LLVM Phabricator instance.

[DWARF][NFC] NFC patch for reverted r342218 (refactoring rangelist handling)
ClosedPublic

Authored by wolfgangp on Oct 22 2018, 6:10 PM.

Details

Summary

The purpose of this patch is not so much a review but to provide an NFC version of the patch that was submitted to create r342218. See the initial review in https://reviews.llvm.org/D51081.

The original patch caused TSan failures and this is an attempt to narrow down the cause.
Basically, the difference from the original patch is that it does not change any of the current format for dumping rangelists.
It is also rebased vs. Dave's latest patch supporting RLE_startx etc.

Diff Detail

Event Timeline

wolfgangp created this revision.Oct 22 2018, 6:10 PM
JDevlieghere accepted this revision.Oct 23 2018, 2:10 PM

I only did a quick scan as it was reviewed before. Is there a particular area I should pay extra attention too? If not this LGTM.

include/llvm/DebugInfo/DWARF/DWARFListTable.h
164

Nit: Ctx?

This revision is now accepted and ready to land.Oct 23 2018, 2:10 PM

I only did a quick scan as it was reviewed before. Is there a particular area I should pay extra attention too? If not this LGTM.

Thanks, this was actually meant for Eric to see if it runs into the same TSan test issues as the previous patch.
@echristo Do you want me to commit this or can you take it separately?

This revision was automatically updated to reflect the committed changes.
echristo added inline comments.Oct 29 2018, 3:37 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

Trying to figure out a way around isDWO as a boolean argument and data member would be great.

dblaikie added inline comments.Oct 29 2018, 3:39 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

Avoiding it being a boolean, or avoiding having the same name for an argument and data member at the same time?

echristo added inline comments.Oct 29 2018, 3:45 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

The former. I'd like to avoid the boolean existing if we can avoid it, and definitely as an argument to the constructor as well.

dblaikie added inline comments.Oct 29 2018, 3:52 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

Ah, OK. In the "boolean arguments are hard to read" (eg: replace them with an enum) or the "this is extra state/information that this part of the code might, hopefully, not need to know"?

wolfgangp added inline comments.Oct 29 2018, 4:08 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

Yeah, it looks like it's always available from the context. If all goes well with this patch I'll try to do this as a follow-on if that's OK with you.

dblaikie added inline comments.Oct 29 2018, 4:42 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

Worth a shot - though one wrinkle: Support for non-split Split DWARF. I know the llvm-dwarfdump support for this is incomplete, but it's something to keep in mind - "isDWO" might not be a constant/globally true thing even for a single object file - it may contain both split and non-split content in the same file.

wolfgangp added inline comments.Oct 29 2018, 4:50 PM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

I had interpreted "isDWO" as something that derives from the section you're extracting from, i.e. when extracting a table from either .debug_rnglists or .debug_rnglists.dwo, so we should be able to treat the presence of the non-split content orthogonally, hopefully.

probinson added inline comments.
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFListTable.h
165 ↗(On Diff #171588)

My impression is that "isDWO" generally guides how to interpret cross-section references, so references from debug_foo.dwo would go to debug_bar.dwo instead of debug_bar. The dumper and verifier don't do cross-file stuff?
The debugger would though... so in the interest of converging the LLVM and LLDB readers, we might want to upgrade the bool to a 3-value enum at some point.

lib/DebugInfo/DWARF/DWARFContext.cpp