Thanks for posting your patch! I will continue reviewing, but here is a first detail I found..

Thanks, yes looks good to me!

Thanks for taking care of reverting.

Thanks, that sounds reasonable. Maybe we can provide some more context on atexit vs. __cxa_atexit in the commit message?

Running check-llvm and check-clang locally found no more failing tests. The issue is not limited to pre-ISel intrinsics (anymore) and I have to re-phrase a few comments. Apart from that, is there any more feedback? Thanks

Add missing objc_sync_exit to fix failing test Transforms/PreISelIntrinsicLowering/objc-arc.ll

Thanks for your feedback.

LGTM. Let's go and land this once Lang's two latest comment got addressed.

This is the best solution I found so far. The funclet bundle operand (aka. token) of the inlinee function will be propagated to the set of intrinsics that is (as of today) subject to pre-ISel lowering. This only applies when targeting WinEH and only for functions inlined into EH funclets.

Limit explicit propagation to actual pre-ISel intrinsics

Overall, I think this looks pretty good! I didn't follow the design discussions closely, so please bare with me in case some of my questions are dumb :-) In a way it makes sure other people will understand your code as well.

Drop GNUstep assertion from getBundlesForFunclet(). Indeed a similar issue was observed before. See: D44640

Fix accidental functional change that failed Clang test CodeGenCXX/microsoft-abi-eh-inlineasm.cpp

Update LLVM CodeGen test for mainline opaque pointers

Just to add some context for other reviewers: We want this code in the header to make sure it's inlined into the backends.

LGTM

LGTM, thanks for working on this!

LGTM, thanks for working on this!

LGTM, thanks for working on this!

LGTM, thanks for working on this!

For illustration, truncations look like this (left: old CodeGen, right: with this fix applied)

My follow-up got delayed, because I hit another bug, which appears to be a regression in release 14. This is why I wrote the tests for release/13.x and I still have to port them back to mainline, so this is *not yet ready to land*. As I don't expect it to cause big changes, however, I though it might be good to update the review anyway.

Clang frontend: check that 'funclet' bundle operands are emitted for Pre-ISel intrinsics

LLVM CodeGen: check that presence of bundle operands avoids truncation

Fix unchecked nullptr compiler crash and assertion

LGTM!

Thanks for your patch. The tests look good and appear to work fine on my machine. Will have a closer look at it towards the end of the week.

The difference between macho backend is that in ELF, the shift value is explicitly given by relocation type. lld generates the relocation type that matches with instruction bitwidth, so getting the shift value implicitly from instruction bytes should be fine in typical use cases.

This is testing the functionality that's wired up in D126287 right? Can we add it there? Thanks

If it's not too much effort, can we add this to the review for the implementation in D126630? Thanks

If it's not too much effort, can we abandon review and add it to the one with the implementation in D126658? You tracked the dependencies here in Phabricator nicely, but it's usually easier for people to see their relation in the git history if it's one commit. All functional changes need testing and so checking them in together is good practice. Thanks!

Good catch!

Kudos, this is a really good first patch. Thanks for working on this sunho! I think this is almost ready to land as is. Just pointing out a few minor details I encountered while reading through.

Thanks for your help! I will work on testing in calendar week 21.

I guess testing must be split in two:

• Clang wants to make sure the "funclet" bundle operand gets emitted for ObjC ARC runtime calls on Windows. Maybe that fits into clang/test/CodeGenObjC/gnu-exceptions.m?
• LLVM wants to check that WinEHPrepare handles these runtime calls correctly. For that maybe I can use llvm/test/CodeGen/X86/win-funclet-cfi.ll as a template and adjust it for the ObjC ARC case?

Thanks for your comments Reid, this was really helpful!

There is a second attempt here, that I am more uncertain about: https://github.com/weliveindetail/llvm-project/commit/93e830eb1b9280d08e3f082b746a8d1343d413ac

Added some more details here on Github:
https://github.com/weliveindetail/libobjc2/commit/cfd3ede57901fa1d294268d8ef2ab992fefa3fb1#r72508265

Thanks for the patch. I can't dig into the details deeper right now, but if the test issues remain I do so next week.

This is from llvm/Support/MathExtras.h right? Can you add it as an explicit include please? Otherwise LGTM.

This patch wasn't adopted. Instead, ORC opted for explicit runtime paths in https://reviews.llvm.org/rG13c8ec44e6385127f4aaba333dcad705dd5888c6

I can't reproduce the original error on mainline anymore. I guess one of the EPC refactors fixed the issue on the way. Thanks

it always fall into OutOfRangeError

Hi Kelvin, thanks for working on this. I will run your patch through my pi tonight and see how I can help.

Eventually this should be fixed with 878060aaf965

Passing a valid function address is the responsibility of the user and laying out argv correctly is an ABI detail. If anything like this fails, we probably have to hang up anyway and send the error with that D111527.

Thanks for confirming the issue. Aiming for a more thorough solution is fine for me. I committed the SimpleRemoteEPC port for the LLJITWithRemnoteDebugging example with ac2daacb310cbb1732de1c139be7a0e8e982169e even though it's not fully functional yet, because it drops the last in-tree dependency to the old OrcRPC implementation. Once we have a fix for the threading issue here, I will re-enable the test for the example (see D110649).

SimpleRemoteEPC port of the LLJITWithRemoteDebugging example landed with ac2daacb310cbb1732de1c139be7a0e8e982169e

Two minor comments. Otherwise LGTM.

Using the plugin's very own DebugObject::finalizeAsync() is not sufficient

FYI Included one more typo fix in ExecutorAddress.h

FYI Included one more typo fix in ExecutorAddress.h (Edit: This was in the other review.)

Issue reported during review D110260. With this fix, nm should return something like this now:

➜ nm bin/LLJITWithExecutorProcessControl | grep return1
0000000100004c10 T _return1

In D110260#3018412, @xgupta wrote:

Yeah we need testcases, now it says

JIT session error: Symbols not found: [ return1 ]
JIT session error: Failed to materialize symbols: { (main, { foo_body }) }
Unable to lazily compile function. Exiting.

Thanks @xgupta for your note! The parameter passed to EPCIndirectionUtils::Create() in the example was referencing a moved-from value. This caused the segfault. Unfortunately, the examples don't have good test coverage so far.

@mgorny I hope it's not breaking your test suites again?

Thanks for working on this. The formatting nits should be straightforward to fix. Apart from that, it would be really nice if the jit-loader_jitlink/rtdyld_elf.test tests could reflect this fix. Otherwise it might regress again any time.

You're right, it's been a bad merge. In the meantime @GMNGeoffrey fixed it with 01b097afd0ea. Sorry for the inconvenience.

@GMNGeoffrey You'er right, thanks for the fix. I've been distracted the whole day.

In D105429#3005522, @xgupta wrote:

It think someone need to accept this revision now to close it.

Ping! I was hoping to land this together with D109522.

Account for modifications in the underlying patch

Account for modifications in the underlying patch && address pre-merge checks