I asked about the rationale behind the 2-PLT scheme to H. J. Lu who proposed the design. I was not convinced one of the reasons he presented, which is that the 2-PLT scheme provides compatibility with old systems, because clearly the 2-PLT scheme breaks compatibility by changing the semantics of the existing .plt section and adding the second PLT. The other reason was more convincing, which is, it was designed with performance in mind, and it was an effort to reduce the size of the hot code path. With the 2-PLT scheme, the second PLT is in theory hot and the first PLT is relatively cold, as the first PLT is used only when a symbol is resolved lazily. That being said, there was not evidence or benchmark results supporting the claim, so I cannot really buy that argument.

Perhaps we should remove the usage of these flags from the Linux kernel even after this patch has landed because they are just dead flags, but this patch per se looks good to me.

In D58484#1407078, @ikudrin wrote:

For me, the code should be, above all, reliable. You can trust it only in that case; otherwise, you never know, when and how it is going to be broken and produce an unexpected result.

I would suggest a simple test for both approaches. What if a developer adds a new diagnostic message?

With D58300 it will probably just work as expected in both modes out-of-the-box. You even don't need to check that it is properly formatted, because most of the formatting is performed in one place, uniformly.

With this patch (D58484) the developer might not be aware of the existence of the --vs-diagnostics switch. Thus, recognizing patterns will not probably be updated and the message generation will be broken for an unpredictable time until someone comes across that new message in Visual Studio.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

• address review comments

In D58102#1403358, @LuoYuanke wrote:

I think SPLT has better compatibility compared to change the original PLT entry. Given the platform is old, and use the old dynamic linker, the new library that build with SPLT can still be linked by dynamic linker. I am not sure if it still works if we change the struct of PLT entry.

LGTM

In D58102#1400315, @xiangzhangllvm wrote:

Hi ruiu, I asked its designer. This is really specified in x86-64 psABI SPEC, Please refer https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf 13.2 DynamicLinking

LGTM

• Simplify

• fix typo

LGTM

I think we need to be very careful how to implement the feature. Currently it is very easy to understand what lld prints out as an error message and how to construct an error message. Basically, what you see in the source code is what you get at runtime. This patch added an abstraction layer between the current code and stderr, and now it is not easy to figure out what is the actual output and how to construct a desired error message.

LGTM

I really don't think we should introduce a new PLT (.splt) if the reason of adding it is to keep the compatibility of the existing .plt format. Second PLT is too complicated and would become a technical debt.

1 Yes, we will disable the feature if not all the input object files contain the .note.gnu.property section, because the CET feature will supervise all the indirect jumps in the program. If one input file do not contain the CET info (flags which is set in .note.gnu.property section's GNU_PROPERTY_X86_FEATURE_1_AND property ), it means it can‘t be check by the CET hardware, so we need to disable the CET feature. This is important to link the non-CET library.

LGTM

LGTM

Can I ask a few random questions to understand the design of the patch?

LGTM

LGTM

If we define a synthetic ARM.exidx section which consumes all .ARM.exidx sections and replace them with itself, we can perhaps remove SHF_ORDER and other features that we add for .ARM.exidx sections. If there's no other use of the features, I'd like to remove them.

We used to handle .ARM.exidx sections as regular sections with a sentinel section that is synthesized and added to the end. We later added code to merge contiguous .ARM.exidx sections, and now this patch is adding sentinels at various points. At this point, maybe it is easier to handle .ARM.exidx as a single synthetic section just like MergeInputSection? That synthetic section removes all input section whose name is .ARM.exidx from the input section list and add itself to the input section. That design gives you more flexibility than the current design, as the intermediate data representation doesn't have to be an InputSection.

LGTM

LGTM

This is a fairly large patch that introduces a completely new feature to lld. Please allow me time to review this patch carefully at high level; I wouldn't like to review the code in detail until high level concerns or questions are addressed. Next time, please consider sending an RFC to express your intent to implement a feature before finishing up your patch. Completing a proof-of-concept is fine, but further development should be done incrementally in my opinion. It is not an uncommon situation in which we have to request a significant rework if a large feature is implemented all at once in a single patch. In addition to that, splitting a patch into a number of incremental patches makes code review much easier.

I personally think that they should update their toolchain so that they can handle the standardized compressed debug section instead of a GNU-extension, or they should disable tests that depends on the GNU extension by examining if -compressed-debug-section=gnu is supported before running the tests. I particularly dislike the fact that the GNU extension uses .z as a prefix for a compressed section, which honestly seems ugly.

Boolean flags usually have the form /foo[:no] in MSVC linker, so I'd propose /demangle and /demangle:no for lld/coff.

Even though I believe what this patch does is correct, it seems really complicated to me, but that's not a fault of this patch. This part of lld code is mind-boggling. At this point, it is perhaps easier to rewrite it entirely than incrementally improving the code quality of this file. I don't think I would do this soon, but that's probably what we should keep in mind.

Submitted this patch and filed a merge request as https://bugs.llvm.org/show_bug.cgi?id=40721.

Just want to know a bit more about background of this change. Are you making an ABI-breaking change, so you are defining a new ABI version number for the AMDGPU target. Is this understanding correct? Is the AMDGPU target migrating to ABI version 2, or will version 1 and 2 co-exist in the future?