- User Since
- Jun 25 2020, 10:59 AM (143 w, 3 d)
Wed, Mar 22
Removs BS isChanged
rebase, use new instead of mmap
From my standpoint this patch seems to be more solid and universal. We don't have to change structure, don't have to create pseudo relocation types on BOLT level and can compose relocations with more then 2 symbols easily if needed (although I'm, not sure if there are such cases). Indeed I was interested in this patch since the first thought was that might be re-used in golang support in couple of places and it seems to be a solution that might help with it. But the only major downside of this solution - we don't have special label relocations for aarch64/x86. But it seems to be reasonable addon and probably worth to speak with community about adding such types to other platforms too.
Tue, Mar 21
Nice! I didn't know about this feature. May I ask you to create dependent review with getComposeOpcodeFor ? Thank you!
Mon, Mar 20
Sorry @sebpop but I don't think it is part of ARM mapping symbols ABI standard. What type of compiler are you using when observing this? Is there any way to create test to reproduce it?
Sat, Mar 18
LGTM, but let's wait next week plz, maybe @rafauler have some other thoughts :)
Fri, Mar 17
LGTM, but small nit - I would swap this new condition with the " if (!Section->isText()) " condition above, since this one is more "univesal" and might appear not inly in text section. And although currently it does not really matter it probably better from the logical standpoint.
Thu, Mar 16
Wed, Mar 15
Tue, Mar 14
Mon, Mar 13
Thank you for this patch :)
Thu, Mar 9
@rafauler Gentle ping
Tue, Mar 7
@Elvina Please check :)
Fri, Mar 3
Tue, Feb 28
Add comments to code
Add static flag to test
Fix last segment size
Mon, Feb 27
@sebpop But it is wrong behaviour. All the symbols after mmaping symbol are considered to be that type before new symbols is located. It is not per-symbol thing
LGTM Thank you!
Well it looks to me that for some reason mapping symbols are corrupted. Basically it is not bolt problem, the only way you can overcome this issue without binary fixing is ignoring these functions manually with skip-funcs option. Even with this patch bolt won't produce working binary anyway.
@maksfb I think you're right, probably mappings symbols has something to do here. Currently islands are not printed, but I think it is a good idea to add such functionality.
@sebpop I was unable to briefly find the sources of "lj_vm_ffi_call" function, probably it is auto generated, maybe it is using the same hack then openssl using .long directive to emit instruction using opcode.. Anyway please try to find the closest $d or $x symbol to the lj_vm_ffi_call function with symbol adress <= lj_vm_ffi_call address. If it is $d it means that the instruction would be interpreted like a data rather then code and we need to find the source of the problem somewhere where lj_vm_ffi_call function is created..
Feb 23 2023
Feb 22 2023
Feb 21 2023
LGTM, please check @rafaelauler comments.
Wow, would definitely take a look to this patch, thank you!
Feb 17 2023
@rafaelauler Yes, I know about that. I don't really use it (although I think we can give it a try), but it seems to be working fine, the test in the RW segment patch shows it :)
Feb 15 2023
Add test. The test is using reorder data pass and .data section which must appear in the writable segment.
@rafauler @maksfb gentle ping. Also please check D144095 D143748 . Thank you!
Feb 14 2023
@rafauler Gentle ping :)
Feb 10 2023
Feb 7 2023
Change patch according to https://reviews.llvm.org/D143390
@maksfb @rafauler please review D143390 :)
Feb 6 2023
@rafauler I've found out that we can make small linker scripts for lld, it uses it as a hint and adds missing sections by it self.
New writable segment creation was re-developed and separate review was created, let's start from there https://reviews.llvm.org/D143390 @maksfb @rafaelauler @Amir
BOLT part LGTM
Feb 2 2023
Feb 1 2023
Overall LGTM, although usually we have a policy to minimise yaml tests as much as we can, removing unneeded for the tests sections, symbols, data & etc.
Jan 28 2023
I'm not sure what causes clang-format to complain about BF.h , locally I'm unable to repeat this.
Anyway I've commented about 3 main problems in this patch. I would be happy to discuss them and submit the golang support patches one-by-one, it would be the most right and simple away I think.
@maksfb @rafauler @Amir
Jan 26 2023
I'm not sure why check-bolt doesn't work for you, it works fine for me..
Plus we've got more complicated tests than this one, probably it is something in env causes the problem. I don't say that there is no problem, but I would say that probably we need another test or maybe pre-built binary in yaml format, we have some tests like that. Also be aware that %cflags contains nostdlib, so no startup files (that seems to have a problem) would be linked in...
Please try make check-bolt. Although test seems to be too easy, I doubt that it could check the problem..
Jan 18 2023
Nice, thank you!
LGTM either, thank you!
Jan 11 2023
Jan 9 2023
Jan 8 2023
Hello @Amir ! As you suggested I'm starting to splitting this patch to the parts. I suggest not to use stack of changes, it would be more convenient for me to post group of changes one-by-one, you can refer to the rest of the changes on this patch. As I understand your idea although smaller changes by it self are not very useful we still can commit them one by one due to the size of this patch. So the first change was prepared for core: https://reviews.llvm.org/D141234 . Thank you!
Rebase & some updates
Jan 7 2023
@maksfb I see, thank you. I assume that the application code becomes locked due to some kind of meta optimisations for the services on the kernel level or interpreter , probably it is not common situation... Because of that I don't really think this munlock call may harm somehow, so LGTM.
Oh, you are totally right about the locks, I've briefly checked that moment and made a wrong assumption, thank you!
As for tryLocks I'm totally agree with you that basically it should have the same behaviour as lock. But I don't see how __bolt_instr_data_dump could be called from signal handler. Basically we've got 2 situation when this function is called - when DT_FINI is called or (my situation) when we've got forked process waiting for the parent to be dead. The DT_FINI (probably) never get called from the signal handler and our fork doesn't have signal handlers.
Plus when the deadlock occurs the parent process is already normally exited at that moment, so no deadlock should be at that moment.. So I'm still confused why it happens sometimes.
Dec 22 2022
Dec 21 2022
The CI lld does not support this relaxation for now, revert test to prebuilt blob.
Thanks @Amir ! For now I would probably just remove this option, I don't think it is really necessary to be honest :)
@maksfb I'm just not completely understand why mlock has something todo with it? Why the binary memory even get locked on startup?
Dec 20 2022
Could you please provide related kernel commit? Thank you!
It seems to be that CI prebuilt lld is a bit out of date and does not support --relax option. I may remove this since it is the default, although I think it is probably good to have one. What do you think @Amir ?
Add lld relax option
Dec 16 2022