Page MenuHomePhabricator

[llvm-objcopy] Delete --build-id-link-{dir,input,output}
ClosedPublic

Authored by MaskRay on Feb 8 2021, 6:49 PM.

Details

Summary

The few options are niche. They solved a problem which was traditionally solved
with more shell commands (llvm-readelf -n fetches the Build ID. Then
ln is used to hard link the file to a directory derived from the Build ID.)

Due to limitation, they are no longer used by Fuchsia and they don't appear to
be used elsewhere (checked with Google Search and Debian Code Search). So delete
them without a transition period.

Announcement: https://lists.llvm.org/pipermail/llvm-dev/2021-February/148446.html

Diff Detail

Unit TestsFailed

TimeTest
20 msx64 debian > LLVM.tools/llvm-symbolizer::coff-dwarf.test
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-symbolizer 0x5009 0x5038 --inlining --relative-address -obj="/mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-symbolizer/Inputs/coff-dwarf.exe" | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-symbolizer/coff-dwarf.test
60 msx64 windows > LLVM.tools/llvm-symbolizer::coff-dwarf.test
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-symbolizer.exe 0x5009 0x5038 --inlining --relative-address -obj="C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-symbolizer/Inputs/coff-dwarf.exe" | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-symbolizer\coff-dwarf.test

Event Timeline

MaskRay created this revision.Feb 8 2021, 6:49 PM
MaskRay requested review of this revision.Feb 8 2021, 6:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 6:49 PM
phosek accepted this revision.Feb 8 2021, 7:52 PM

LGTM

This revision is now accepted and ready to land.Feb 8 2021, 7:52 PM

No longer used by Fuchsia and not used elsewhere.

How can you be sure this isn't used outside Google and the LLVM upstream repo?

I'm not necessarily opposed to this (it's not in our downstream users' documentation, and we don't support it in our downstream toolchain), but what are you going to do if this breaks another user when this gets released in LLVM 13? If there's a reliable alternative, that's fine by me, but I'd encourage it to be in the commit message, and probably also in release notes, so that existing users (if there are any), can know what to do instead.

llvm/tools/llvm-objcopy/ELF/Object.h
446

This change seems unrelated?

MaskRay marked an inline comment as done.Feb 9 2021, 10:57 AM

No longer used by Fuchsia and not used elsewhere.

How can you be sure this isn't used outside Google and the LLVM upstream repo?

I'm not necessarily opposed to this (it's not in our downstream users' documentation, and we don't support it in our downstream toolchain), but what are you going to do if this breaks another user when this gets released in LLVM 13? If there's a reliable alternative, that's fine by me, but I'd encourage it to be in the commit message, and probably also in release notes, so that existing users (if there are any), can know what to do instead.

I am figuring out whether we can retire llvm/runtimes/llvm-strip-link.in

The functionality has llvm-readelf+ln replacement. It was implemented in llvm-objcopy for its convenience. Debian Code Search tells me that no external open-source project uses the functionality.

If it is used by some proprietary users, I think the best is an llvm-dev announcement that the options will be deleted if nobody speaks up. If nobody responds, we will delete the options.

llvm/tools/llvm-objcopy/ELF/Object.h
446

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.

phosek added a comment.Feb 9 2021, 9:19 PM

I am figuring out whether we can retire llvm/runtimes/llvm-strip-link.in

I don't think we need it anymore for the Fuchsia toolchain build as we have since implemented a more generic mechanism providing similar functionality, but it may be also worth checking whether there are other users on llvm-dev.

The functionality has llvm-readelf+ln replacement. It was implemented in llvm-objcopy for its convenience.

There are many features in LLVM tools you could in replace with other tools and some bash/Python glue, but usually at the expense of efficiency. In Fuchsia we rely heavily on build ID and for every binary we build, we want produce both stripped and unstripped version and link them into the .build-id directory. The problem with using llvm-objcopy+llvm-readelf+ln wrapped in a script is that it's more expensive. This may not be an issue if you do this once, but in our build it happens several hundred times and that's where the overhead becomes noticeable which is why we implemented this feature in llvm-objcopy.

The reason why I'm not pushing against the removal of this feature is that we've since found out that it's insufficient on its own and we needed additional logic, but prototyping new features in llvm-objcopy is quite difficult, so we have since implemented a custom downstream tool that we use instead. I'm hoping we could eventually upstream that functionality, but either as a separate tool or as an alternative frontend for llvm-objcopy akin to llvm-strip/llvm-install-name-tool.

Debian Code Search tells me that no external open-source project uses the functionality. If it is used by some proprietary users, I think the best is an llvm-dev announcement that the options will be deleted if nobody speaks up. If nobody responds, we will delete the options.

I'd check with llvm-dev, Debian code search may not be sufficiently exhaustive.

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.

If the workaround is spelt out for people, I'm not opposed, as already mentioned.

llvm/tools/llvm-objcopy/ELF/Object.h
446

I'd agree with that, except I can't see where Contents was previously being used?

MaskRay marked an inline comment as done.Feb 10 2021, 9:59 AM

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. For APIs, the policy is clear "downstream is on their own". Upstream contributors consider usage and can be polite by giving transition period.
This patch is removing options, so the bar is probably higher. If proprietary users want to keep features (I am a bit unclear how we should draw such a line), they should engage with the community.
Otherwise, unused features are subject to removal.

If the workaround is spelt out for people, I'm not opposed, as already mentioned.

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. For APIs, the policy is clear "downstream is on their own". Upstream contributors consider usage and can be polite by giving transition period.
This patch is removing options, so the bar is probably higher. If proprietary users want to keep features (I am a bit unclear how we should draw such a line), they should engage with the community.
Otherwise, unused features are subject to removal.

APIs are a bit different than the public documented command line argument (& other behavior) support for these tools. I think it's important we strive to maintain compatibility - though, yes, it varies depending on the tool, use case, etc. In this particular case if we have communication from the folks who wanted/implemented the feature, and now don't want/use it & no one else has said anything about it - I'm OK with removing it, documenting it's been removed & if some folks turn up later to say they want it, we could discuss adding it back in, potentially.

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.

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?)

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.

MaskRay updated this revision to Diff 323210.Feb 11 2021, 7:31 PM
MaskRay edited the summary of this revision. (Show Details)

add release note
improve summary

MaskRay edited the summary of this revision. (Show Details)Feb 11 2021, 7:32 PM

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.

llvm/docs/ReleaseNotes.rst
127

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.

128

Dumb question maybe, but what's the trailing _ for?

llvm/tools/llvm-objcopy/ELF/Object.h
446

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.

MaskRay updated this revision to Diff 323417.Feb 12 2021, 11:22 AM
MaskRay marked 2 inline comments as done.

comments

MaskRay edited the summary of this revision. (Show Details)Feb 12 2021, 11:23 AM
MaskRay added inline comments.
llvm/docs/ReleaseNotes.rst
128
llvm/tools/llvm-objcopy/ELF/Object.h
446

The original patch adding the featured move the variable.

446

I checked that the variable can be moved without the patch. So I dropped it.

jhenderson accepted this revision.Feb 15 2021, 1:46 AM

LGTM!

llvm/tools/llvm-objcopy/ELF/Object.h
446

Thanks. Feel free to make a separate commit to move it if you haven't already.

MaskRay edited the summary of this revision. (Show Details)Feb 15 2021, 11:07 AM
This revision was landed with ongoing or failed builds.Feb 15 2021, 11:17 AM
This revision was automatically updated to reflect the committed changes.