Address comments

@rafaelauler Thanks, I've decided to add proper description since currently I don't really see any examples when removeConditionalTailCalls would be necessary. We can discuss it in discord if it will be comfortable for you or @maksfb .

Address comments

dbgs->outs in veneer pass

Run veneer ellimination pass before instrumentation.
We would like to remove veneers asap and before instrumentation pass, since we want to record call to the real function, not it's veneer.
@maksfb @rafauler gentle ping

@maksfb @rafauler gentle ping

rebase

Uncomment __builtin_unreachable() branch handling.
@maksfb as a tmp solution I purpose to add extra check in unreachable handling. I hope we will able to move elliminate this code here oneday,
but for now it should work fine. Also as I see there is some other code that I think we need to be refactore also, e.g. removeConditionalTailCalls
in postProcessCFG. Ideally we want to move all code transformations to the passes, since at least golang support needs it. Thank you!

Add test checks

ADd test description

Abandon the diff for now

Yes, you're right, I didn't notice that it is not iotas object aligned to .align 8, but the aligning zeroes.

Add AlignCIMaxBytes option, address comments

@tschuett Thank you for your comment. Yes, I know, but currently bolt uses uint16_t for alignment in its sources, this is out of scope of this patch to refactor and change this.

Gentle ping

LGTM

address comment

In D127223#3564746, @maksfb wrote:

Some of the passes that calculates tentative layout like LongJmp and Golang are expecting that only functions with valid index will be located in hot text section.

Do these passes care about functions written to cold text at all? I wonder if we need to assign indices to all functions regardless of the profile.

Please fix the test

Fix comment

Overall LGTM

LGTM Thanks @treapster

LGTM
AFAIU this might be the first time where the skipRelocationProcess that I'm using for arm might be used for x86

LGTM Thanks!

-

@Amir Consider to add puts to stub.h and include the header

@rafaelauler Gentle ping

@tschuett please checkout the answer above, I'm not sure if i will use llvm-objdump or will use other methods right now.

@tschuett What do you mean by add go to llvm-objdump?
@ayermolo 'm not sure if I will be able to cover it compeletely or even all corner cases, probably only basic moments, but I will try to. As a first step I suggest to discuss technical point of the patch, there are some places that surely should be redone, I need some suggestions on this :) I don't think it will be fast process due to the patch size, my personal workload & etc,, but during that time I will try to add some test cases :)
UPD Could somebody please point me out what is the problem with git-clang-format stage? Locally It does not show any problem and I don't see any comments in the diff too, no idea what it is complaining about :(

clang format warn

Some places are explained and marked by me that needs extra attention or some suggestions for re-implemetation

@rafaelauler gentle ping

Remove all changes but refactoring

Great, thanks!

@rafaelauler As I understand we will need to keep dynamic relocations in binary function/data like we do it for static relocation and emit labels in the right places. Let's discuss it in discord in the nearest time if you have any suggestion. I don't really like the binary function part of the code too (initially I've made the review NFC). As for this review I suggest to separate the container type refactoring and to think about the dynamic relocations emission separately, what do you think?

Nit

Add test

@rafauler I've decided to add extra arg for relocation emission, it is easy to add and easy to revert in case we won't need it anymore

@rafauler Well, the time regression is about couple of %, even before patch I got up to 390sec, and 390+ sec after. The memory regression is negligible. It is possible to do what you're suggesting, but it looks to me like extra complication that personally me would like to avoid. I really don't think it is worse it, but it is discussible question

@rafaelauler Gentle ping

@rafaelauler Gentle ping

Update

@Amir I think you need to rebase the patch, I will check it on aarch64 then :) Thank you!

@rafauler I've checked the biggest binary I have. As for the time after couple of launches I see 380-390 sec before patch and 390-400sec after patch, which I assume is just a negligible difference. As for the memory the I've got 38279868kb before and 38348116kb after (+68mb or +0.2%) which is small amount too and matches yours results. So I assume it's a reasonable change with an acceptable overhead :)

nit
@rafauler It was my fault, not all tests should be OK, sorry for bothering :)

@Amir Thank you very much, it works now, submitting the patch now :)

clang-format

Fix unreachable.s

@rafaelauler Right now I've got only ubuntu aarch64 :( I will try to check it on x86 ubuntu locally soon, although I don't really see the reason for such a strange behaviour..

@rafaelauler I've tried couple of times, but the result seems to be the same...