remove IsChanged

fix tests

Removs BS isChanged

rebase, use new instead of mmap

rebase

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.

Nice! I didn't know about this feature. May I ask you to create dependent review with getComposeOpcodeFor ? Thank you!

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?

LGTM, but let's wait next week plz, maybe @rafauler have some other thoughts :)

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.

Address comments

Fix merge

Thank you for this patch :)

@rafauler Gentle ping

@Elvina Please check :)
LGTM

Thank you!

Add comments to code

Add static flag to test

Fix last segment size

@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!

LGTM

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..

LGTM thanks

LGTM, please check @rafaelauler comments.

Wow, would definitely take a look to this patch, thank you!

@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 :)

nit test

clang-format

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!

clang format

nit
@rafauler Gentle ping :)

Change patch according to https://reviews.llvm.org/D143390
@maksfb @rafauler please review D143390 :)

nit

@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.

Address comments

Gentle ping @maksfb @rafauler

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

Gentle reminder

LGTM

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.

nit

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

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..

Small changes

Nice, thank you!

LGTM either, thank you!

Thanks

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

@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.

@michoecho
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.

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?

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

Replace test

Address comments