[PATCH] D144083: [JITLink] Initial AArch32 backend

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h:171
+/// Helper function to read the initial addend for Data-class relocations.
+std::optional<Expected<int64_t>> readAddendData(LinkGraph &G, Block &B,
+                                                const Edge &E);
----------------
tschuett wrote:
> Why do you wrap `Èxpected` with an `optional`?
Hi Thorsten, thanks for your note! The idea was that each of the `readAddendX()` functions can fall through to the error handling at the end of `readAddend()`. `std::nullopt` indicated that it didn't handle the case, while `Expected` alone would either be a `llvm::Error` or a `int64_t`.

However, it made me think again about the kind of error we face when one of the `applyFixupX()` or `readAddendX()` fails: An error for unsupported edge kinds is raised when populating the link-graph in a much earlier link phase. This is an error in the input object and thus propagated as `llvm::Error`. Here, at fixup-time, we should assume that all edge kinds are valid. If we still encounter an invalid one, this is an implementation error and should trigger an assertion failure/`llvm_unreachable`.

In my new patch, I implemented this AND dropped the `optional`s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144083