We may think in generalities, but weliveindetail
User Details
- User Since
- Jul 10 2018, 11:23 AM (208 w, 20 h)
Thu, Jun 30
Thanks for posting your patch! I will continue reviewing, but here is a first detail I found..
Sat, Jun 25
Thanks, yes looks good to me!
Tue, Jun 21
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?
Mon, Jun 20
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.
Thu, Jun 16
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.
Wed, Jun 15
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
Tue, Jun 14
Update LLVM CodeGen test for mainline opaque pointers
Mon, Jun 13
Just to add some context for other reviewers: We want this code in the header to make sure it's inlined into the backends.
Tue, Jun 7
LGTM
Jun 5 2022
LGTM, thanks for working on this!
LGTM, thanks for working on this!
LGTM, thanks for working on this!
LGTM, thanks for working on this!
Jun 3 2022
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
Jun 1 2022
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.
May 30 2022
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!
May 25 2022
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.
May 3 2022
Thanks for your help! I will work on testing in calendar week 21.
May 2 2022
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!
Apr 29 2022
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
Jan 30 2022
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.
Jan 26 2022
This is from llvm/Support/MathExtras.h right? Can you add it as an explicit include please? Otherwise LGTM.
Jan 5 2022
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
Nov 11 2021
Oct 25 2021
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.
Oct 23 2021
Eventually this should be fixed with 878060aaf965
Oct 11 2021
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.
Sep 29 2021
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
Sep 28 2021
Sep 27 2021
Two minor comments. Otherwise LGTM.
Using the plugin's very own DebugObject::finalizeAsync() is not sufficient
Sep 23 2021
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
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.
Sep 22 2021
@mgorny I hope it's not breaking your test suites again?
Sep 20 2021
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.
Sep 17 2021
Ping! I was hoping to land this together with D109522.
Sep 13 2021
Account for modifications in the underlying patch
Account for modifications in the underlying patch && address pre-merge checks