User Details
- User Since
- Apr 18 2013, 12:16 AM (305 w, 2 d)
Yesterday
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.
LGTM
Thu, Feb 21
LGTM
LGTM
Wed, Feb 20
LGTM
LGTM
- address review comments
Tue, Feb 19
LGTM
LGTM
- Simplify
- fix typo
LGTM
Fri, Feb 15
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.
Thu, Feb 14
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
Wed, Feb 13
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.
Tue, Feb 12
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?