cc @kyulee

Add test

I work with Shoaib and this looks like a good solution for us! Thank you!

This diff is a ~0.5% size regression under -Oz when building with -fstack-protector. Is this reasonably expected? And one that should be expected given that it's considered to be a correctness fix? cc @smeenai

In D136774#3887013, @smeenai wrote:

This makes sense to me, but we should probably add a test case?

I've found internally that the value here has heavy variance. ~35 is best for uncompressed code on aarch64 Linux with .eh_frame. ~5 seems best for armv7 Linux with .eh_frame. If you're using compression then many outlinings are wasteful compared to compressibility and various project-specific results come out with this parameter finding optimal values up through many hundreds.

@MaskRay I haven't quite figured why yet, but I'm hitting a runtime assert failure due to this diff in our internal CI builds. I'll try to reduce this, but it's failing on our internal codebase so I can't share right away. But, to summarize:

One interesting note Shoaib and I just discovered is that ~16.5% of the libraries in one of our major apps have .text sections starting on the first page of the dso. Placing hot sections at the front of the .text section would give us a slice of a "free page" that, with the current algorithm, would just be cold code that's less likely to be used. I imagine this isn't a substantial difference but it's probably slightly in favor of placing hot code first here.

😢

Fix typo

rebase

In D126828#3551712, @smeenai wrote:

Ugh, something went tragically wrong with Phabricator's diff display here for some reason. https://reviews.llvm.org/file/data/in2yitrbhop3rtr6ypig/PHID-FILE-qctkyjkcaytlepm3qxe6/D126828.diff looks much better ... could you confirm that it matches the changes you have?

fix context

Yay, thanks Jonas!

In D123272#3434918, @ChuanqiXu wrote:

When post a revision, it would be better to contain more context of the diff page. Generally, this could be done by adding the argument -U9999 when you generates the diff.

update from repo

Sorry to bump such an old diff, but this seems relevant to have in tree. I was looking up the enable-ipra flag and this is the only reasonable reference for it.

@tstellar This could get picked to llvm-13. It's a bug that was first found with us rolling out llvm-12 internally and is still present in 13.

Add test

Hey Fangrui, is there any reason this couldn't extend to armv7?

The comment at the top says this is the master but this def seems right. @MaskRay did the compiler-rt change so I'll just tag him in.

Fix check in test

Address Fangrui's comments

Address Fangrui's comments

I'd completely rewrite the description. Consider this:

Yea, I figured that this was likely a fragile ordering issue. I was thinking about an approach but didn't know enough about lld outside of the standard LTO code path, thus figured I should ask here first. Thanks for the feedback!

Add test case

The isDefined() requirement is to make version-script-weak.s work: a STB_WEAK lazy symbol, if not fetched, should be treated similar to an undefined weak symbol. An undefined symbol should not have non-VER_NDX_GLOBAL versionId values.

The summary says !isLocal but the code says !isLazy. I'm assuming the code is correct.

An older version of the patch had the vtables marked "hidden" in the LLVM assembly. My point was that clang would have given them a non-public vcall_visibility in that case.

Well right now they are marked with public visibility, which is the same thing as not having any vcall_visibility. Since they are marked in the IR as having hidden visibility, clang presumably should have marked them with a more restricted vcall_visibility (linkage unit level).

Do you use a local: version node in a version script to make vtable symbols local in a -shared link? LTO does not know the effective binding has become local in that case and can lose devirtualization opportunities.

Thanks Fangrui! A cherry-pick and test and local run makes it seem like it works.

I see - so just to confirm, when compiling it isn't clear that these symbols are have internal or hidden visibility, but only during linking? Because otherwise clang should already have applied an appropriate vcall_visibility that allows WPD.

To me this is WAI. Why is "config->shared" true for your bitcode module? This should only affect when using the linker flags that assert you have whole program visibility during the link, which isn't true for a shared library and its symbols.

@tejohnson I'm not sure this change is working correctly -- either that or my builds are messed up.

Thanks, Jonas! This generates bindings but they aren't entirely functional as it generates type annotations like "lldb::SBDebugger" which Python LSPs can't figure out. I ended up adding -py3 and then running some sed post-processing to convert "lldb::\(.*\)" to \1 etc: https://github.com/lanza/lldbpybind. That lldb.py works pretty well with pyright getting type info. Figuring out how to teach the build system + swig to generate proper type annotations might require a similar post processing step.

LGTM, I had to do this to fix it locally and can confirm it works.