[PATCH] D106624: [DWARF] Don't process .debug_info relocations for DWO Context

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo marked 6 inline comments as done.
ayermolo added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:390
 
+  enum class ContextType { ALL, DWO, MONOLITHIC };
+
----------------
dblaikie wrote:
> dblaikie wrote:
> > This name might be a bit vague - and I don't think LLVM usually uses all-upper for enum constants. So maybe "SplitOrObjectSections" with "{Both, DWO, Object}"? I'm not really sure - open to better names.
> Ping on this. (and maybe "NonSplit" rather than "Object" would be more clear - but finding a more specific name than "ContextType" is probably the important part of making this more self-descriptive/less vague)
How about this?
Thinking is what we really care about is whether to process or not relocations. With warning being consolidated to one case (dwo relocations are present), doesn't seem we need to distinguish between various cases.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1742-1746
+          HandleError(createError(
+              "Creating DWO Context with relocation section for " + Name));
+        else if (Type == DWARFContext::ContextType::ALL &&
+                 Name.contains(".dwo"))
+          HandleWarning(createError("Relocation " + Name + " exists"));
----------------
dblaikie wrote:
> ayermolo wrote:
> > dblaikie wrote:
> > > ayermolo wrote:
> > > > dblaikie wrote:
> > > > > Any thoughts/details on why one is an error and the other is a warning? (since they're about the same content, I think?)
> > > > > 
> > > > > Hmm, also it might not be invalid to encounter relocation sections in a dwo file in general - that's the split-dwarf=single case, isn't it? (ie: in split-dwarf=single, we'd be in this codiepath for ContextType::DWO, but the dwo file is really the .o file, so it'll have relocations for the non-.dwo sections - though they can/should be silently ignored, but they aren't an error/problem)
> > > > My original thinking was if consumer explicitly goes through the DWO context path there is expectation that DWO sections won't have any relocations, and should fail early, and warning might get ignored/missed. The "ALL" path is less critical. It's used for things such as llvm-dwarfdump where it's not necessary for it to fail and warning is more visible. 
> > > > Re-thinking about it now, it doesn't really seem like a good argument. So maybe just keep it as a warning for both?
> > > > Also there is a bug. It should be:
> > > > 
> > > > ```
> > > > if (RelocatedSection != Obj.section_end() && Name.contains(".dwo")) {
> > > >         if (Type == DWARFContext::ContextType::DWO)
> > > >           HandleError(createError(
> > > >               "Creating DWO Context with relocation section for " + Name));
> > > >         else if (Type == DWARFContext::ContextType::ALL)
> > > >           HandleWarning(createError("Relocation " + Name + " exists"));
> > > >       }
> > > > ```
> > > > So dropping the difference:
> > > > 
> > > > ```
> > > > if (RelocatedSection != Obj.section_end() && Name.contains(".dwo")) {
> > > >         if (Type == DWARFContext::ContextType::DWO || 
> > > >             Type == DWARFContext::ContextType::ALL)
> > > >             HandleWarning(createError("Relocation " + Name + " exists"));
> > > >       }
> > > > ```
> > > > 
> > > > Or maybe just to simplify the whole thing and remove enum and 
> > > > 
> > > > ```
> > > > if (RelocatedSection != Obj.section_end() && Name.contains(".dwo"))
> > > >             HandleWarning(createError("Relocation " + Name + " exists"));
> > > > ```
> > > > 
> > > > If we are removing the code that handles the relocations for dwo sections, and establish that this is not a valid scenario, then there is no real reason to distinguish what context this is. Since even if they are present we will print a warning, and output won't be correct.
> > > > If we are removing the code that handles the relocations for dwo sections, and establish that this is not a valid scenario, then there is no real reason to distinguish what context this is. Since even if they are present we will print a warning, and output won't be correct.
> > > 
> > > Almost - but I think you want to skip /all/ relocations when you're in a DWO context, right? (for split-dwarf=single - where you're parsing the .dwo file (which is really the .o file) and don't want to handle all the non-dwo sections, etc because you know its only the dwo parts that are relevant) so you'd still need the context to implement that, I think?
> > Derp moment on my part. The enum is still necessary. 
> OK, so coming back to this code - seems like we could error any time we see relocations for .dwo sections though - without checking the "ContextType". But, separately, we should check the "ContextType" and if it's DWO, no relocations should be processed, yeah? (that latter part seems to be the code a few lines lower, at 1749 \/)
> 
> So this warning/error code could be simplified to:
> ```
> if (RelocatedSection != Obj.section_end() && Name.contains(".dwo"))
>   HandleWarning(createError("Unexpected relocations for dwo section " + Name));
> ```
> 
> Something like that?
Yep, yep, exactly.


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-rela-dwo.s:1
+# Test objective to verify warning is printed if DWO secton has relocations.
+#
----------------
dblaikie wrote:
> dblaikie wrote:
> > This test seems a bit long (I see it's got structure types? That seems unnecessary for this purpose) - maybe a simple/single file with "extern int i; int i = 3;".
> Ping on this - still seems a bit long/convoluted.
OK, let me see if I can modify this/regenerate a new one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106624/new/

https://reviews.llvm.org/D106624