[PATCH] D96310: [llvm-objcopy] Delete --build-id-link-{dir,input,output}

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D96310#2557890 <https://reviews.llvm.org/D96310#2557890>, @MaskRay wrote:

> In D96310#2556172 <https://reviews.llvm.org/D96310#2556172>, @jhenderson wrote:
>
>> In D96310#2554489 <https://reviews.llvm.org/D96310#2554489>, @MaskRay wrote:
>>
>>> In D96310#2553465 <https://reviews.llvm.org/D96310#2553465>, @jhenderson wrote:
>>>
>>>> FWIW, I suspect there are many users of LLVM out there who don't monitor llvm-dev, although an email like you sent certainly can't hurt.
>>>
>>> I think we cannot and should not care about such users.
>>
>> Really? What about all the people out there who use systems where the LLVM toolchain is the default? Do you really expect every user who downloads and installs LLVM to have to monitor llvm-dev? I would think that llvm-dev is for developers who develop LLVM primarily, not for general day-to-day end users of the tools. For example, I don't pay much attention to the cfe-dev mailing list, yet I use clang in my Linux environment all the time. If an option was removed from clang at some point, and I happened to have been using it, I'd expect this change to at least be documented in the release notes, along with an alternative suggestion to allow me to solve the problem that I wanted to achieve (I'd still be somewhat annoyed that my. All I'm asking for is the workaround to be explained in the release notes or something else that is publicly visible, so that a user who is impacted by this has a clear path forward.
>
> (Does your advice that we could add that simple llvm-nm option and later delete it https://reviews.llvm.org/D83152#2556189 conflict with the opinion here?)

A fair comment.  In my opinion, no, because the reason for deleting the llvm-nm option would be because we decided to mirror a new GNU option which served the same purpose (possibly in addition to something else). In such a case, the migration path is obvious (change the option name in your build script), although it probably should still be documented in release notes for the same reasoning as why I think outlining the migration path in relation to this patch is important.

> For `--build-id-link-{dir,input,output}`, it is a pretty niche use case. It solved a problem which was traditionally solved, just providing a direct & fancy alternative. We have confidence that it can be unlikely used by dark web users.
> For the problem, I could mark the options deprecated and let llvm-objcopy emit warnings - I don't use that because that is unnecessary and just complicates things. Yes, I'll note down the options in the release notes.

Yeah, like I've said, I've got no issue with the option being removed immediately. Just letting end users know what to do in the (hopefully unlikely) case they are using it is all I care about.



================
Comment at: llvm/docs/ReleaseNotes.rst:127
 
+* ``--build-id-link-{dir,input,output}`` have been deleted.
+  (`D96310 <https://reviews.llvm.org/D96310>`_)
----------------
I see what you're saying here, but perhaps "The options --build-id-link-{dir,input,output}" etc would be slightly better. My first reading of that was that you're referring to one option, which clashed with the use of "have" :)

I've seen references to `llvm-readelf` and `ln` in the review comments, but it's still not entirely clear to me how to use them to do the same thing. Just a couple of sentences saying the steps to perform to do the same thing either here or in the patch description would be nice.


================
Comment at: llvm/docs/ReleaseNotes.rst:128
+* ``--build-id-link-{dir,input,output}`` have been deleted.
+  (`D96310 <https://reviews.llvm.org/D96310>`_)
+
----------------
Dumb question maybe, but what's the trailing `_` for?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:446
 
+  ArrayRef<uint8_t> Contents;
+
----------------
jhenderson wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > This change seems unrelated?
> > This is natural after the last public user of Contents is deleted.
> > 
> > It does not seem necessary to defer it to a subsequent clean-up.
> I'd agree with that, except I can't see where Contents was previously being used?
Ping this? I have no issue with it being moved, but if it wasn't being used publicly before this patch, it probably should be a separate change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96310