I tested on x86-64 ubuntu 22 and confirmed it builds.

Thanks Simon, I think this patch makes a nice improvement to the behavior of objdump. I'm not the code owner of these disassembly tools, but the patch looks good to me. I would rather wait for the input of whoever is more familiar with objdump, though.

Rebasing

Can we add more testing that exercises the new code paths being added here?

Thanks!

I'm also curious if this adds runtime overhead by parsing LSDA two times per function. Can we measure that using a large binary as input?

Excellent work Huan! I have a few design suggestions below. Let me know what do you think.

Those should be available in your build folder, here:

I agree, erasing functions in that pass seems hacky. Thanks.

LGTM thanks!

There's a memory leak the two tests:

Thanks for working on improving hugify!

Looks good, let me kick off tests

I think that works. Feel free to reopen or create a new review, whichever is easier. CI runs:

The cause of the crash caused by this diff is because this diff moves processing of interprocedural references outside the disassembly loop.

LGTM

Thanks!

Hi @yota9, I reverted this commit because it is causing BOLT to crash in one of your internal binaries, let me work a bit more on it to understand what is happening before you land it again. Thanks!

Thanks

LGTM

LGTM

Rename typo in title BOTL -> BOLT

Thanks Huan for all the work put in this diff. I have a few more comments below.

That's awesome, thanks

Do you have a testcase?

FYI if you just want to test, I think you can publish this as "draft" using "arc diff --draft"

LGTM

LGTM

LGTM

LGTM

LGTM

It's a bit confusing to call a function named "stable_sort", which is a well-known C++ stdlib function, but we're actually calling stable_sort from LLVM and not from stdlib. I've tried looking up in LLVM repo and there are all sorts of usages of is_contained, for example: some of them use llvm:is_contained, others use is_contained directly. What is your take on this? Should we call llvm::stable_sort, or just stable_sort?

LGTM

LGTM

TBH it's not like code that makes weird layout assumptions is really important/buying us much performance anyway... so we might just replace it if we can., at the source, or just skip it / preserve it untouched in the original section.

The way we support openSSL users for x86 (or users of any assembly-written libs that have layout assumptions, for that matter) is usually via -skip-funcs.

For reference in this discussion, offending code is here: https://github.com/openssl/openssl/blob/master/crypto/sha/asm/keccak1600-armv8.pl#L87

Thanks @yota9 for working on this! I have some questions below.

Thanks for reviewing @yota9 and thanks for your contribution @treapster. Do you need me to commit this?

LGTM

Hi @treapster. The purpose of emitting data in the middle of the code section in RISC machines is to have it close to code that refers to it, so as to use less instructions to address this data. Otherwise, you would do it x86-way and just emit all data in the data sections, which could be far away (depends on how large is your text section). It's possible to argue that it's better for the cache hierarchy to emit data far away, and that for x86, since there is no penalty to address far-away data, it's a bad idea to emit data in the middle of the text section. The reason would be to compact code close together to use less pages in iTLB -- here, having non-executable bytes in .text pages is just wasted space from the point of view of i-cache.

LGTM, thanks!

Btw, the stashed changes can't be recovered by any of the commands listed in your print statement, right? You would need to run git stash apply

Conceptually, the problem is that we need a full cross-compiler to build to a specific target (linux), but we only have the compiler, not the libs. That's why I thought it would make sense to restrict to environments where we are not cross compiling to a different system. Looking at "pie.test", for example, we are apparently building a .cpp file, which is even worse, as it requires libstdc++. Somehow I want to give test writers the idea that it is not OK to build C++ files unless you are not cross-compiling.

In D126933#3555080, @Amir wrote:

I've recently added some magic to pass linux target triple to cflags in bolt/test/lit.local.cfg:

host_linux_triple = config.target_triple.split('-')[0]+'-linux'
common_linker_flags = '-fuse-ld=lld -Wl,--unresolved-symbols=ignore-all'
flags = f'--target={host_linux_triple} {common_linker_flags}'

This would take the arch from target triple (x86_64 or aarch64) and append -linux. I didn't test it on Windows but it works on macOS. Therefore I'm reluctant to add system-linux to these tests that would restrict them to linux.

Can you please share what's passed as cflags on windows? Let's try to fix it. Our tests don't depend on system headers, so there's a good chance they would work even on windows.

(I'm talking about a separate test repo, btw. The in-tree test should still be fixed individually for each diff to avoid breaking bisecting tools)

We should time it to land in the same day and then land changes to tests that compensates for the aggregate of all changes. Regarding the order, I don't have any preference, as long as both patchsets hit the repo in the same day.

Btw from BOLT perspective this diff LGTM. Regarding the changes to X86 target, those look minimal and rather benign to me. It is apparently using tablegen info (GET_INSTRINFO_OPERAND_TYPE) that is not used by any other backend that is in-tree, but TableGen supports generating this info and it is even in their testsuite. Even if it has a bug because it hasn't been used, I think it makes sense to try for us as it obviously won't break any existing code as we are the only users of this information. We should wait for someone responsible for the X86 target to approve this change, though.

Let me try to elaborate on why this diff is important and has functional changes.

No, this diff is not NFC. It's actually pretty impactful (and thanks for working on this Amir).

You're right that it does result in functional changes.

LGTM

@spupyrev I don't remember if you have commit access to the LLVM repo. If you don't, let me know, and I'll commit this once you are done with modifications and give me a green signal.

LGTM

Thanks! LGTM pending the test fix

Okay, looks good to me