[PATCH] D144083: [JITLink] Initial AArch32 backend

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h:231
+  /// Name of the object file section that will contain all our stubs.
+  static StringRef getSectionName() { return "S__STUBS"; }
+
----------------
We use `$__STUBS` elsewhere, rather than `S__STUBS`.

As a side note: I threw a `$` in here to make a clash less likely, but I don't think it's actually an illegal symbol character in ELF or COFF so we should probably pick some universal prefix going forward. Maybe just `__llvm_jitlink`?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp:78-89
+const char *getEdgeKindName(Edge::Kind Kind) {
+  auto ELFType = getELFRelocationType(Kind);
+  if (!ELFType)
+    return getGenericEdgeKindName(Kind);
+  StringRef Name = object::getELFRelocationTypeName(ELF::EM_ARM, *ELFType);
+
+  // FIXME: In order to match the LinkGraph::GetEdgeKindNameFunction interface,
----------------
This should return the name of the JITLink edge enum, rather than the ELF relocation. That's not a blocker for landing though.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/aarch32.cpp:133
+/// followed by bytes A+3, A+2 in the second halfword (Lo).
+class ThumbRelocation {
+public:
----------------
Could this be two types -- `ThumbRelocation` and `MutableThumbRelocation`, with `ThumbRelocation`s constructible from `MutableThumbRelocation`s?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/aarch32.cpp:244
+      return makeUnexpectedOpcodeError(G, R, Kind);
+    if ((R.Lo & FixupInfo<Thumb_Call>::LoBitNoBlx) == 0 &&
+        (R.Lo & FixupInfo<Thumb_Call>::LoBitH) != 0)
----------------
peter.smith wrote:
> sgraenitz wrote:
> > peter.smith wrote:
> > > Looking at the stub generation code, if there is a Thumb_Call to an external edge, then a stub with LoBitH will be set will be created. This won't be a problem if the code-generator never generates BLX to an external symbol, but would be if it does.
> > Yes, I didn't catch a case like this yet. But I am also not sure I fully understand: If codegen wrote a BLX, then it would be T2 right? The docs say LoBitH must be empty here:
> > ```
> > if CurrentInstrSet() == InstrSet_ThumbEE || H == '1' then UNDEFINED;
> > ```
> > 
> > What is the stub generation code you mean? The one here in JITLink?
> Yes, the code in visitEdge, quoted here. If there is a BLX instruction with a Thumb_Call relocation where the target isn't defined, it looks like a stub will be created.
> 
> ```
> bool visitEdge(LinkGraph &G, Block *B, Edge &E) {
>     if (E.getTarget().isDefined())
>       return false;
> 
>     switch (E.getKind()) {
>     case Thumb_Call:
>     case Thumb_Jump24: {
>       DEBUG_WITH_TYPE("jitlink", {
>         dbgs() << "  Fixing " << G.getEdgeKindName(E.getKind()) << " edge at "
>                << B->getFixupAddress(E) << " (" << B->getAddress() << " + "
>                << formatv("{0:x}", E.getOffset()) << ")\n";
>       });
>       E.setTarget(this->getEntryForTarget(G, E.getTarget()));
>       return true;
>     }
>     }
>     return false;
>   }
> ```
> I would not expect a code-generator to generate a BLX in this situation as it has no idea whether the target is Arm or Thumb so I would expect a BL. It is possible to write in assembly language but I don't know how well that needs to be supported in a JIT.
> 
> In static linkers an R_ARM_CALL (Arm state) or R_ARM_THM_CALL (Thumb state) relocation can be associated with a BL or BLX. As the static linker always knows the (Arm/Thumb) state of the destination it can write a BL instruction if both source and target are the same state or a BLX if they are different. This property permits code-generators to only use BL with R_ARM_CALL/R_ARM_THM_CALL, with BLX reserved for assembler. It also permits Thumb code to reuse Arm stubs.
> 
> I don't know how often this would happen in a JIT though.
We definitely want to support this eventually, and the JIT linker should have all the information that it needs to do so (though it's possible that we'll need to fix some plumbing in the symbol tables to transfer the thumb bit through).

I don't think we need this in the initial commit though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144083