[PATCH] D128382: [LLD] Two tweaks to symbol ordering scheme

Fri Jul 8 12:02:47 PDT 2022

smeenai added a subscriber: ruiu.
smeenai added a comment.

In D128382#3639411 <https://reviews.llvm.org/D128382#3639411>, @yozhu wrote:

> In D128382#3639367 <https://reviews.llvm.org/D128382#3639367>, @MaskRay wrote:
>
>> Right. The code can spend fewer lines to address the issue. This is one of the moments I just wanted to commandeer the patch and satisfying the user needs instead of wasting more and more time arguing.
>
> Indeed we have spent quite some time arguing about this.  I don't agree with you on "corner case", "not useful", or "fewer lines of code"; and I really don't understand why you push back so hard on this change.  Before you requested another revision one week ago the only remaining unsettled point was whether we need to track each input section because you think their attributes cannot be reliably tracked, then I showed you with an example that this is not the case.  Then I think it should be all good then.
>
> It is simply fixing a flaw in the original ordering scheme and the new additional logic doesn't add too much complexity or overhead compared to what is already there for mixing cold and hot contributions.  Please don't abuse the administrator rights.

I think it'd be helpful for us to step back a bit. We all want to maintain the overall health and cleanliness of LLVM and LLD while also being able to make changes that help with the use cases we care about. We have different aspects of code we value more highly (e.g. simplicity vs. thoroughness), but all of us ultimately want the project to function as best as it can.

@ruiu always valued code simplicity highly for LLD, and I think @MaskRay has similar sensibilities :) Edge cases are important, of course, but LLD has historically considered it okay to not handle some rare ones until we have a proven need. The current version of the patch reads straightforwardly enough to me, but we can test it vs. a version where we only check if the output section is executable and see if it makes a difference for any of our internal libraries, to confirm if we have a use case for one check over the other.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128382/new/

https://reviews.llvm.org/D128382