Page MenuHomePhabricator

[lldb/DWARF] More DW_AT_const_value fixes
ClosedPublic

Authored by labath on Aug 21 2020, 6:22 AM.

Details

Summary

This fixes several issues in handling of DW_AT_const_value attributes:

  • the first is that the size of the data given by data forms does not need to match the size of the underlying variable. We already had the case to handle this for DW_FORM_(us)data -- this extends the handling to other data forms. The main reason this was not picked up is because clang uses leb forms in these cases while gcc prefers the fixed-size ones.
  • The handling of DW_AT_strp form was completely broken -- we would end up using the pointer value as the result. I've reorganized this code so that it handles all string forms uniformly.
  • In case of a completely bogus form we would crash due to strlen(nullptr).

Depends on D86311.

Diff Detail

Event Timeline

labath created this revision.Aug 21 2020, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2020, 6:22 AM
labath requested review of this revision.Aug 21 2020, 6:22 AM
labath updated this revision to Diff 287023.Aug 21 2020, 6:27 AM

Actually, keep the old const_value test, but give it a bitfield-specific name. Testing bitfields+const_values is still interesting.

Thank you finding these additional fixed, it looks good but I don't know some of the details as well as Adrian so I would prefer he gives the LGTM.

This looks nice! I'm somewhat suspicious that the new test doesn't specifically test the union case of the old test, but I'm assuming that would still work and your simpler tests covers the same code?

aprantl accepted this revision.Aug 21 2020, 1:07 PM

LGTM with that caveat

This revision is now accepted and ready to land.Aug 21 2020, 1:07 PM

This looks nice! I'm somewhat suspicious that the new test doesn't specifically test the union case of the old test, but I'm assuming that would still work and your simpler tests covers the same code?

For a while I did want to just delete that union+bitfield test, but I eventually concluded that it is interesting to keep it. This patch keeps the original tests, only it renames it to a less generic name.

Sounds good!

This revision was landed with ongoing or failed builds.Aug 26 2020, 4:24 AM
This revision was automatically updated to reflect the committed changes.