https://github.com/llvm/llvm-project/commit/29f6c0fdfeb48f5f766d2ba02014358df69e8170 should avoid the issue

I can fix it now

Thanks LGTM! Let me double-check the tests on my machine and land this for you.

Thanks! Let me double-check the tests on my machine and land this for you.

Thanks for the updates! Can we align the assembly in the test a little more with the (now) existing ELF_static_thumb_reloc.s? That may avoid future divergence. Otherwise, this looks good to me.

Thanks for the updates! Please have a look at one last detail in the test.

I think this can land as soon as the test fits with the (now) existing ELF_static_arm_reloc.s and your previous patch is in.

Can we align the test with the (now) existing ELF_static_arm_reloc.s please? The rest should be ready to land I think.

Thanks for the fixes. This looks good to me! I assume all affected tests still pass. So, let's get this landed.

In D157540#4636960, @sgraenitz wrote:

So, the Thumb_Jump24 relocation is the one that supports conditional branches and Thumb_Call is the one that should have this check

Sorry for being late at the party. I was too busy in the last weeks to approach non-trivial reviews. This is looking really good already. Thanks for your help @lhames!!
Here are my 2 cents. Unfortunately, I cannot make inline comments anymore.. might be due to the move from Pharicator to GitHub PRs. I will do my best to keep this readable.

Ok, I finally double-checked it and you are right. Bit 12 must be set and not clear! Can we rename the field to LoBitUnconditional and change the condition into if (!(R.Lo & FixupInfo<Thumb_Jump24>::LoBitUnconditional))?

Backport for 17.x release: https://github.com/llvm/llvm-project/issues/64800

Thanks for your quick feedback everyone!

The issue was reported in https://github.com/llvm/llvm-project/issues/64582

Thanks! LGTM. Yes let's keep this on hold for now and attempt the RuntimeDyldChecker change first.

LGTM, thanks! Should I land it on your behalf?

Is the test actually working right now? Since we always use indirection stubs and we only have a Thumb implementation right now, my quick assumption was that the TargetSymbol should always have the ThumbSymbol flag set (and thus always bail out). Maybe I find the time in the next days to look into it myself and give some informed feedback.

Thanks, good catches! I will double-check the conditional bit change and let you know.

Looks great, thanks! I think at some point we should add a .thumb section with .thumb_func functions to our tests, so we get coverage for the code that handles ARM-Thumb transitions. However, I guess we need ARM stubs for it first?

This is looking very good already! Nice idea to check ARM and Thumb in one test. I am not sure, however, if it's worth the effort. They both produce R_ARM_ABS32 and R_ARM_REL32. The only difference is the alignment and here Thumb alone would be sufficient:

➜ llvm-objdump -r ...
RELOCATION RECORDS FOR [.text]:                   RELOCATION RECORDS FOR [.text]:
OFFSET   TYPE                     VALUE           OFFSET   TYPE                     VALUE
00000004 R_ARM_ABS32              target          00000002 R_ARM_ABS32              target
00000008 R_ARM_REL32              target          00000006 R_ARM_REL32              target
       ^ ARM is on a 4-byte boundary                     ^ Thumb is 2-byte

Wow, that's a lot of typo fixes! Thanks!

This review should be closed

LGTM too. Is this still up-to-date?

Great! This LGTM. Let's keep the review open for a few days and see if there is more feedback.

That's an excellent patch! I didn't expect it could be so compact. Thanks for working on this! Please find below 2 inline comments on details.
On July 25th release/17.x will branch away. It would be great to land this before and have it in the upcoming release.

In D107087#4473634, @v.g.vassilev wrote:

@sgraenitz, could we move forward with the patch as is?

@justice_adams The patch linked above fixes the issue for GCC. Does it work for you as well? Also, there is a ticket here that looks very similar: https://github.com/llvm/llvm-project/issues/63204

Scratch that. It turned out to be some side-effect of link order. I still don't quite get it, but the anchors are not necessary. Sorry for the noise.

All modified classes have out-of-line virtual methods (or at least dtors) already. Nevertheless, without these additional anchors the vtable symbols appear to be missing from libMyShared.so. Eventually, I am not certain why this is necessary. I don't see similar issues for libLLVMOrcJIT.a and they are built and linked the same way.

This issues popped up with GCC 12 as well after https://github.com/llvm/llvm-project/commit/47f5c54f997a59bb2c65abe6b8b811f6e7553456

Rebase

Rebase and re-run pre-merge checks

This patch adds test coverage for existing functionality. It was ready to land 6 weeks ago. There was plenty of opportunity to raise concerns and discuss details over the last 2 months of review. I assume the topic is just too niche to attract a lot of attention.

In D141215#4272538, @lhames wrote:

using regular global variable instances to manage the storage on the executor side, an extended MemoryAccess interface to read/write the value from the REPL side when needed (e.g. for printing), and emitting glue functions to pass the variable's value in to callers.

One more guess: Could this function possibly reside in a runtime library for MinGW? lli has --orc-runtime but it's not in use implicitly. As far as I know, we only support MSVC. Would MinGW support be interesting? Do we have anything in place? Maybe @sunho knows more about it. Thanks!

In D149996#4325838, @mstorsjo wrote:

Conversely, an MSVC-built lli.exe runs the test just fine even

Rebase and double-check default config: GNUstep disabled, objc-gnustep-print test unsupported

Address feedback and rebase

In D146058#4305046, @labath wrote:

That's not entirely what I was referring to. What I fear is the following situation. A random developer makes a random patch that happens to break gnustep support. That developer cannot debug that issue locally (cannot or doesn't know how to build gnustep from source), so someone has to help him figure out the problem. I don't want to be the person doing that. :)

Ping

Thanks. LGTM

In D141824#4274461, @argentite wrote:

We should probably also address the lack of linker issue as well. Should we go for a precompiled dynamic library file? There seems to be some "precedent" of this in other tests.

LGTM

Thanks. Maybe you can fix this one detail and add a description like "Most of Orc and JITLink are movinng away from JITTargetAddress and use ExecutorAddr instead"? Otherwise LGTM.

Great. Thanks!

Double-checked and found source locations on Raspberry Pi 3b with this patch applied https://github.com/llvm/llvm-project/issues/61948#issuecomment-1508305542
Is there more feedback on this? It would be nice to land it soon, i.e. before EuroLLVM =)

This looks a lot better already than the implementation I know from Cling. Well done!

I think it's a very good practice that objects in moved-from state will only ever get destroyed. While the move ctor itself has no preconditions and thus might be fine to call again, the implementation of consumeError() does inspect the object. This adds an unnecessary risk for UB. Please simply check the log state and call either fmt_consume() or consumeError(). Thanks.

Thanks! LGTM

Thanks for your feedback

Include R_386_NONE for error reporting

Addressed feedback and added handling for R_ARM_REL32. I didn't see a real-world case for it yet. Hope it's ok to have the error reported for the time being.

Add error reporting for R_ARM_REL32 relocations

Rename new function to ApplyELF32ABS32RelRelocation()

The plan sounds very reasonable. PowerPC support is one of the last missing pieces for feature parity with RuntimeDyld. Speaking for the JITLink code, this looks excellent of course.

In D141824#4247891, @zhuhan0 wrote:

This seems to be the first clang test that involves actual linking.

First of all, yes the existing code is incorrect. Thanks for looking into this. However, I am afraid this isn't the right solution.