Page MenuHomePhabricator

[lldb][DWARF5] Enable macro evaluation
ClosedPublic

Authored by kpdev42 on Jul 19 2022, 1:04 AM.

Details

Summary

Patch enables handing of DWARFv5 DW_MACRO_define_strx and DW_MACRO_undef_strx

~~~

OS Laboratory. Huawei RRI. Saint-Petersburg

Diff Detail

Event Timeline

kpdev42 created this revision.Jul 19 2022, 1:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 1:04 AM
kpdev42 requested review of this revision.Jul 19 2022, 1:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: lldb-commits, Restricted Project, sstefan1. · View Herald Transcript
tschuett added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h
27

If you would use a class instead of the struct, you could hide the member variables.

The class variant is slightly more verbose than the struct.

clayborg added inline comments.Jul 19 2022, 10:48 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp
95–97

This enum should be different otherwise this code change does nothing.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp
22

I would make this DWARF64 compatible.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h
28

"cu_offset" sounds like it is actually the offset of the compile unit. This is actually the compile unit offset for the string data for the compile unit in the string index section right? Maybe a name like "cu_str_offset" or "cu_strx_offset" would be a better name?

29

Can "data" ever be NULL? If not, then change this to a reference and add a constructor that inits the reference?

31

if you are going to keep "data" as a pointer, shouldn't this function also check "data" against nullptr?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1239–1244

If we want to change "data" to be a reference, we will need a constructor:

DWARFStrOffsetsInfo str_offsets_info(dwarf_cu->GetStrOffsetsBase(), symfile.GetDWARFContext().getOrLoadStrOffsetsData());
clayborg added inline comments.Jul 19 2022, 11:02 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp
96

Never mind me! Just checked the LoadOrGetSection definition. They were all different for other sections, so I thought they needed to be different...

kpdev42 updated this revision to Diff 451364.Aug 10 2022, 12:09 AM

Address review notes

kpdev42 edited the summary of this revision. (Show Details)Aug 25 2022, 12:37 AM
clayborg accepted this revision.Sep 8 2022, 10:58 AM
This revision is now accepted and ready to land.Sep 8 2022, 10:58 AM
This revision was automatically updated to reflect the committed changes.

Out of curiosity — did you get an email notification from the bot?

Out of curiosity — did you get an email notification from the bot?

Hi, yes, I will add a priority to these notifications to react faster, sorry

Out of curiosity — did you get an email notification from the bot?

Hi, yes, I will add a priority to these notifications to react faster, sorry

Great. I was asking because we had issues with the bot not sending these out recently.