This is an archive of the discontinued LLVM Phabricator instance.

[DWARFYAML][debug_info] Rename some mapping keys. NFC.
AcceptedPublic

Authored by Higuoxing on Aug 5 2020, 3:33 AM.

Details

Summary

This patch renames some mapping keys:

AbbrOffset -> DebugAbbrevOffset
AddrSize -> AddressSize
AbbrCode -> AbbrevCode

Diff Detail

Event Timeline

Higuoxing created this revision.Aug 5 2020, 3:33 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Higuoxing requested review of this revision.Aug 5 2020, 3:33 AM

What's the motivation for doing this?

What's the motivation for doing this?

We should make these mapping keys' name consistent with the spec.
The spec uses 'address_size', 'debug_abbrev_offset', 'abbreviation code' to refer these fields.

I see the point, but we don't do it for all fields in other contexts, and I have some mild concerns that DebugAbbrevOffset is unnecessarily verbose (I'd think AbbrevOffset would be sufficient. Perhaps it would be best to draw in one or two others? @JDevlieghere / @labath, any thoughts?

I see the point, but we don't do it for all fields in other contexts, and I have some mild concerns that DebugAbbrevOffset is unnecessarily verbose (I'd think AbbrevOffset would be sufficient. Perhaps it would be best to draw in one or two others? @JDevlieghere / @labath, any thoughts?

Yeah, I'm ok with these names. I would like to hear more people's ideas!

CC @jankratochvil who might also have thoughts on this topic :-)

Sorry about the delay, I was OOO.

I think that making these consistent with the DWARF spec is a good idea. It's true that this makes DebugAbbrevOffset a bit longish. I could also live with just AbbrevOffset, but I think the longer version is also fine for two reasons:

  • I expect most of this stuff to be created by copy-pasting
  • Ideally, I would want to avoid explicitly specifying this field in most cases, and instead rely on some sort of symbolic references between the sections (as discussed in the other reviews)
jhenderson accepted this revision.Aug 27 2020, 12:44 AM

Okay, LGTM. I don't mind either way, and I suspect with the offset field becoming optional soon, it's unlikely to appear frequently, so the verbosity is a non-issue then.

lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp
53

Nit: the value here is misaligned.

llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp
2296

Could you fix this linter nit, whilst you're here, please?

This revision is now accepted and ready to land.Aug 27 2020, 12:44 AM