This broke the ppc64le-sanitizer builderr: https://lab.llvm.org/buildbot/#/builders/19/builds/3565. Will fix it.

In D98916#2678792, @peter.smith wrote:

Thanks for the update LGTM. It sounds like you have all the cases covered. Apologies for the noise.

Fix a typo.

In D98916#2673445, @peter.smith wrote:

I have a one major concern, and a number of minor nits that could easily be addressed prior to commit if necessary.

The major concern is that if the intent is to just permit expressions for Arm state LDR instructions (addition of one fixup) then a lot of code in the AsmParser looks like it is permitting expressions for Thumb and Thumb2 LDR instructions that wouldn't use the one new fixup. Can you double check the predicates to make sure that they apply to the instructions you want to permit expressions on? It is possible I've got something wrong though. If I'm right then where we would have had errors had someone use an expression then we may get an internal error or crash as the backend won't be able to handle the expression. My suggestion would be to take out from ARMAsmParser all the code permitting exceptions except for the ARM instructions that support the fixup. Adding support for expressions to all the ARM and Thumb instructions is possible but that would probably be better done over several patches. If I'm wrong please ignore me and sorry for the noise.

Address comments.

I'm ok with this, and thanks for taking the time to work on it. It would be good to collect another LGTM from another reviewer before submitting.

Only accept MCExpr in the affected instructions.

Relanded without -fno-initegrated-as case at https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5.

We can probably remove the -fno-integraetd-as case.

Update tests.

Update tests to address comments.

Address comments.

In D98916#2637305, @peter.smith wrote:

The fixup code looks reasonable to me as it is mostly reusing the same logic as the pc-relative fixup. The names are internal to LLVM so there isn't any particular standard. I think this fixup would correspond to the relocation R_ARM_ABS12 https://github.com/ARM-software/abi-aa/blob/master/aaelf32/aaelf32.rst#relocation-codes so it may be worth using fixup_arm_ldst_abs_12 but that is only a suggestion. I wouldn't expect the assembler to use the relocation as the range is so short that any attempt to use it would likely result in an out of range error.

Address comments so far.

I'm not sure if this would be a big issue but restoreStatOnFile also runs on the split dwo file, and will keep its ownership with this change. I don't think GNU assembler does that currently. Also you could delete the code added in https://reviews.llvm.org/D98511 for passing user and group ID since they are no longer needed, assuming the new behavior is what we want.

Add period after a comment.

Remove unncessary CHECK-NOT.

Update test cases.

Skip range check of MCBinaryExpr in b.w instead of any non-constant expressions.

Skip range check of MCBinaryExpr instead of any non-constant expressions.

This is created based on the discussion at https://reviews.llvm.org/D97501#inline-914358.

Remove encodings from the tests.

Update the test cases.

Landed at https://reviews.llvm.org/rGc2a84771bb63947695ea50b89160c02b36fb634d.

Update a comment.

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

Gotcha. Thanks. I've updated the message.

In D93881#2558654, @MaskRay wrote:

Overall, LGTM. I am fine with fchown, but the description needs a lot of changes to proceed.

Thanks for this patch. You mentioned the following issue in one of your comments on https://reviews.llvm.org/D93881. IIUC, this patch seems to not have taken care of this issue.

Reuse the result of stat instead of calling it on the output file again.

Check the user of the temporary file instead of the output file to decide the user that invokes the tool.

Limit the patch to root user.

Add trailing comma to comments.

Address @manojgupta's comments.

Update the ownership of the temporary file, instead of the overwritten file.

The test failure turned out to be a false positive. Relanded the change on https://github.com/llvm/llvm-project/commit/9dfeec853008109b1cbe926c22675c96226040d9

I have mentioned that the GNU objcopy/strip smart_rename still has vulnerability and the owner preservation does not work for non-root (CAP_CHOWN) users.

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

In D91460#2496849, @jrtc27 wrote:

Which GDB tests? How are they broken? I'm extremely surprised by the given this patch implements what binutils has done for a very long time (only .asciz/.string have changed behaviour in binutils, and those are not modified by this patch here so should not have regressions).

In D93881#2483674, @rupprecht wrote:

From a user standpoint, I agree that "objcopy foo.o" should not be changing the ownership of a file, regardless of the implementation (which, in the case of llvm-objcopy, is getting it "wrong" compared to GNU objcopy, because we use temporary file and rename it at the end, without fixing ownership). Sounds like there's agreement there.

GNU objcopy before 2.36 had the symlink attach vulnerability. The patch would introduce the similar vulnerability to llvm-objcopy.

Address some comments.

In D91460#2461621, @jrtc27 wrote:

Looks good to me, let's see what the others think.

Update the test case.

Just want to follow up and see if anyone might have any thoughts on the current implementation of this patch. Otherwise, is it good to submit the patch? @jrtc27 @rnk @nickdesaulniers @MaskRay

Updated a test case.

Update the syntax in the comment.

Address @jrtc27's comment. paseOp function now skips all the spaces between string arguments until it hits any comma or end of statement. For now we will limit the support to .ascii only.