[PATCH] D129936: [JITLink][COFF][x86_64] Reimplement ADDR32NB/REL32.

Wed Jul 20 15:17:23 PDT 2022

sgraenitz accepted this revision.
sgraenitz added a comment.
This revision is now accepted and ready to land.

Alright! Thanks, I didn't see that.

Form my side this looks all really good. I am sure you know best whether it's ready to land or if you want Lang to review as well.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:140
-    }
-
     // FIXME: Skip debug info sections
----------------
And then maybe:
```
// FIXME: Revisit crash when dropping IMAGE_SCN_MEM_DISCARDABLE sections
```

================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:325
+    if (SymbolSet.empty())
+      continue;
     jitlink::Block *B = getGraphBlock(SecIndex);
----------------
sunho wrote:
> sunho wrote:
> > sgraenitz wrote:
> > > How does your patch affect symbol sets so that they might be empty now?
> > It's actually the side effect of the last patch (not dead stripping asso
> The truncated comment I submitted is wrong comment. It's because of the change in this patch (ignoring IsDiscardable) This was needed to prevent a crash that happens in a hello world obj file. If you want me to, I can separate out that change as an another patch with a testcase.
No worries, you don't need to split that out. However, keeping empty symbol sets around might be surprising for the reader (and in fact that might be solved at some point?). Do we always expect them to be `IMAGE_SCN_MEM_DISCARDABLE`? Then why not:
```
assert((cantFail(Obj.getSection(SecIndex))->Characteristics & 
        COFF::IMAGE_SCN_MEM_DISCARDABLE) != 0 && "Revisit after fixing crash")
```

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

https://reviews.llvm.org/D129936