We're unnecessarily parsing and dumping macinfo/dwo section contents, even when sections are empty.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
Does this matter? "parsing" an empty section would be successfully parsing zero contributions & dumping nothing. That seems like a fine implementation & avoids needing this special case to check the section size first.
Comment Actions
Sort of, consider this for a moment --
First we try to parse macinfo --- which for now is empty. assume
then Macro is constructed due to this call.
const DWARFDebugMacro *DWARFContext::getDebugMacinfo() {
if (Macro)
return Macro.get();
DWARFDataExtractor MacinfoData(*DObj, DObj->getMacinfoSection(), isLittleEndian(),
0);
Macro.reset(new DWARFDebugMacro());
Macro->parse(*this, MacinfoData, &Offset);
return Macro.get();Now after wards we try to parse/dump macro section using mostly similar function then,
const DWARFDebugMacro *DWARFContext::getDebugMacro() {
if (Macro) -- since this is constructed previously, function call will return from here it self. hence no contents dumped
return Macro.get();
DWARFDataExtractor MacroData(*DObj, DObj->getMacroSection(), isLittleEndian(),
0);
...BTW, in current implementation, My checks are heavy or the empty Macro construction/parsing/dumping is heavy ??
Comment Actions
These should be stored in two separate variables - if the current "getDebugMacinfo" stores its state in a member called "Macro", then probably worth renaming that member variable to "Macinfo" so that the future "getDebugMacro" can store its state in a member named "Macro" and there's no confusion about which one is which.
By the sounds of it, storing them in the same variable would break dumping of mixed binaries that link together DWARFv4 using debug_macinfo and DWARFv5 using debug_macro, so that's no good.
Comment Actions
OK, so this is a subtle divergence between how debug_macinfo is parsed and debug_macro is parsed, even though they share some code. With macinfo the looping happens here in this function and with macro the looping happens in the caller to this in dumpMacroSection.
Please fix this so macro and macinfo are handled in similar/the same way - both should build a single DWARFDebugMacro object representing all the contributions, then dump them together from there. (look at the way the existing macinfo support is written in DWARFContext - starting there/making the macro support similar)
That's your remark on D73086, Please correct me if I've mis-understood the context here-
here if (Macro) -- Macro is std::unique_ptr<DWARFDebugMacro> Macro for dumping, as of now it's used for macinfo/dwo parsing/dumping. You mentioned both should build a single DWARFDebugMacro object representing all the contributions so that's what I'm trying to achieve here, macinfo is not present then try constructing macro if present.
Comment Actions
I meant each (macro and macinfo) should build a single DWARFDebugMacro of their own (so two in total).
The current code in that review makes macro and macinfo handling very different - the macinfo handling builds one DWARFDebugMacro that contains all the contributions from the debug_macinfo section. Whereas the macro handling builds one DWARFDebugMacro for each contrtibution from the debug_macro section.
Instead, what I'm suggesting is that the debug_macro support should work in the same way as the debug_macinfo section - the debug_macro support should build one DWARFDebugMacro object (its own, not shared with the debug_macro support) representing all the contributions from the debug_macro section - instead of building one per contribution.