[PATCH] D131347: Set up basic infrastructure for ELF/i386 backend support in JITLink .

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Thanks very much for working on this Kshitij -- this looks like a great start to the i386 backend!

I have a couple of minor comments -- could you address them and update the review? Once that's done I think that this is ready to land. :)



================
Comment at: llvm/lib/ExecutionEngine/JITLink/i386.cpp:1-9
+//===---- aarch64.cpp - Generic JITLink aarch64 edge kinds, utilities -----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
----------------
Comments should be updated to i386.


================
Comment at: llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s:3
+# RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t/i386_simple.o %s
+# RUN: llvm-jitlink %t/i386_simple.o; result=$?; [ $result -eq 42 ] || exit 1
+
----------------
Tests should be `-noexec` (except in special circumstances): We want them to be able to run on architectures other than the target.

I would simplify to:
```
# RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t.o %s
# RUN: llvm-jitlink -noexec %t.o
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131347