We may think in generalities, but weliveindetail
- User Since
- Jul 10 2018, 11:23 AM (271 w, 5 d)
Wed, Sep 13
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.
Fri, Sep 8
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.
Thu, Sep 7
Thanks for the fixes. This looks good to me! I assume all affected tests still pass. So, let's get this landed.
Wed, Sep 6
Mon, Sep 4
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))?
Aug 18 2023
Backport for 17.x release: https://github.com/llvm/llvm-project/issues/64800
Thanks for your quick feedback everyone!
Aug 17 2023
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.
Aug 13 2023
Aug 11 2023
LGTM, thanks! Should I land it on your behalf?
Aug 9 2023
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
Aug 6 2023
Wow, that's a lot of typo fixes! Thanks!
Jul 28 2023
This review should be closed
LGTM too. Is this still up-to-date?
Jul 20 2023
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.
Jul 6 2023
Jul 5 2023
Jul 3 2023
May 31 2023
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.
May 17 2023
This issues popped up with GCC 12 as well after https://github.com/llvm/llvm-project/commit/47f5c54f997a59bb2c65abe6b8b811f6e7553456
May 16 2023
Rebase and re-run pre-merge checks
May 15 2023
May 14 2023
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.
May 8 2023
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!
Apr 29 2023
Rebase and double-check default config: GNUstep disabled, objc-gnustep-print test unsupported
Address feedback and rebase
Apr 28 2023
Apr 25 2023
Apr 18 2023
Apr 17 2023
Apr 15 2023
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.
Apr 14 2023
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 =)
Apr 13 2023
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 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.
Apr 6 2023
First of all, yes the existing code is incorrect. Thanks for looking into this. However, I am afraid this isn't the right solution.