Page MenuHomePhabricator

Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children
ClosedPublic

Authored by jankratochvil on Oct 16 2018, 5:23 AM.

Details

Summary

As @aprantl suggested I have merged DWARFDebugInfoEntry's m_empty_children into m_has_children.
m_empty_children was implemented by rL144983. I do not see why @clayborg made it a separate flag, from some point of view it is sure cleaner but technically I do not see its purpose.
I have checked all calls of HasChildren() that it should not matter to them. The code even wants to know if there are any children - it does not matter how the children presence is coded in the binary.
The only problematic may be operator== (and !=) although that one is currently only used by my lldbassert() in my bugfixed D53255.
This patch sure has no regressions on Fedora 28 x86_64.
I do not need/request this patch, just that @aprantl asked about it.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

jankratochvil created this revision.Oct 16 2018, 5:23 AM

m_has_children was used to represent what was in the DWARF in the byte that follows the DW_TAG. m_empty_children was used for DIEs that said they had children but actually only contain a single NULL tag. It is fine to not differentiate between the two.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
281–282

We might want to move m_has_children where m_empty_children used to be and give a bit back to m_abbr_idx by increasing DIE_ABBR_IDX_BITSIZE by 1. We have a few bits to spare in m_sibling_idx since it is the number of dies to advance by to get to the sibling. Since DIEs at the very least are a few bytes, having a 2GB advance instead of 4GB is plenty.

282–284

This comment is wrong. The DWARF encodes if a DIE has children in the .debug_info, but we may override this value if the DIE only contains an NULL terminating DIE.

It should read something like:

// If it is zero, then the DIE doesn't have children, or the 
// DWARF claimed it had children but the DIE only contained 
// a single NULL terminating child.
jankratochvil marked 2 inline comments as done.Oct 16 2018, 10:20 AM
jankratochvil added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
282–284

I cannot say it was wrong but I agree your rewording is much better.

jankratochvil marked an inline comment as done.

See inlined comment.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
281–282

Might be worth changing these to be uint16_t, removing DIE_ABBR_IDX_BITSIZE and changing any code that was using that to make sure that the value is <= UINT16_MAX:

uint16_t m_abbr_idx;
uint16_t m_tag;

Done the conversion of bitfield to uint16_t.

jankratochvil marked an inline comment as done.EditedOct 16 2018, 12:05 PM

Also changed assert()->lldbassert() for m_abbr_idx 16-bit overflow as that could be a tough bug to catch if it ever happens.

clayborg accepted this revision.Oct 16 2018, 1:20 PM

Looks good. Nice cleanup.

This revision is now accepted and ready to land.Oct 16 2018, 1:20 PM
This revision was automatically updated to reflect the committed changes.

Looks good. Nice cleanup.

Thanks for the cleanups and fixes.