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

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added inline comments.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106624