[PATCH] D93881: [llvm-objcopy] preserve file ownership when overwritten

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D93881#2558677 <https://reviews.llvm.org/D93881#2558677>, @jcai19 wrote:

> In D93881#2558654 <https://reviews.llvm.org/D93881#2558654>, @MaskRay wrote:
>
>> Overall, LGTM. I am fine with fchown, but the description needs a lot of changes to proceed.
>
> Thanks!
>
>> For one, the fix advice in https://reviews.llvm.org/D93881#2483542 needs to be taken, `objcopy xxx -o yyy` does not work.
>
> Oh i've updated my commit message locally a while ago, I did not realize this has not been updated in Phabricator. Will definitely update it.

If you use `arc diff 'HEAD^'`, you need `--verbatim` to amend the description.

>> You can mention that as of binutils 2.36, GNU strip calls chown(2) for `sudo strip a` and `sudo strip a -o a`, but not sudo strip a -o b or sudo strip a -o ./a.
>>
>>> The discrepancy prevents Chrome OS from migrating to llvm-objcopy and llvm-strip as Portage (the build system) builds binaries following recommended steps documented at the man page of GNU strip as follows,
>>
>> This part should be rephrased. I think whatever llvm-objcopy/llvm-strip resolution is, Portage may need to migrate away from in-place strip as well @mgorny @robertu94, like https://git.archlinux.org/pacman.git/commit/?id=88d054093c1c99a697d95b26bd9aad5bc4d8e170
>> To prevent pessimistic double write behavior with binutils 2.37.
>>
>>> With llvm-strip and llvm-objcopy, the last two steps change ownership of binaries to root and cause them to become inaccessible to the intended user.
>>
>> The question is about when root strips that is intended to have different owner/group, after atomic rename, should root restore owner/group?
>> I have asked many folks for their opinions. The result is contentious. It can be argued either way. In particular, I got objection like:
>> "the question is not 'how to justify fchown' but 'why justify fchown'" "if someone wants to sudo strip, let them do so and deal with the consequences"
>>
>> I finally swing to fchown because the implicit intention to keep the file accessible by original owner/group.
>> The argument is stronger than "because doing this patch makes us more compatible with GNU, so we do it." While generally it makes sense to keep compatibility when I can do that for free,
>> the binutils behavior is quite problematic. Do we want to take its new behavior? D96308 <https://reviews.llvm.org/D96308> I have thought very hard and finally think "no".
>>
>> This does mean unfortunate platform differences and slightly inconsistent behaviors, but we are going to take it.
>
> Will address the rest.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881