[llvm-objcopy] Change handling of output file permissions

[abrachet]

Summary

Address bug 42082 where files were always outputted with 0775 permissions. Now, the output file is given either 0666 or 0777 if the object is executable.

[abrachet]

I have a number of tests failing, currently 8. I am working through them but wanted to initially get the patch looked at.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

209–210

I removed the check to PreserveDates because we need to stat the file either way now.

[jakehehrlich]

This looks good to me but please have someone else accept first as well.

[jakehehrlich]

llvm/test/tools/llvm-objcopy/ELF/same-permissions.test

3

Actually does stat work like this on windows? Try and confirm before landing so that you don't have to revert.

[abrachet]

Fixed failing test cases

[abrachet]

llvm/test/tools/llvm-objcopy/ELF/same-permissions.test

3

I don't believe it will work on Windows, hopefully the requires system-linux is ok but if not we can just remove the test case

llvm/test/tools/llvm-objcopy/ELF/strip-all-and-remove.test

2

I was having some permissions problems with %t1 so I am just preserving %t's permissions

[abrachet]

Do not stat stdin

[abrachet]

After looking at it more closely, file_status default constructor will set permissions to unknown permissions. So I will need to change how I deal with stdin input. It seems to me that we have two choices, either just give default permissions when reading from stdin, or get the file's type to see if it is executable or not.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/same-permissions.test

3

There is a "stat" in the GnuWin32 tools, which I have on my machine. However, at least the version I have doesn't recognise the "--printf" option. It has another option called --format which looks to do the same thing. With that change, the test actually passes.

llvm/test/tools/llvm-objcopy/ELF/strip-all-and-remove.test

2

It seems odd you're having problems in this test only. There are plenty of other tests that do this sort of copy too.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

243

Nit: this should probably have a blank line before it.

Also, are there any test cases that have llvm-objcopy write to stdout/read from stdin? If not, it would probably be worth adding one, possibly in a separate patch.

[abrachet]

llvm/test/tools/llvm-objcopy/ELF/strip-all-and-remove.test

2

It was certainly strange. %t had your normal 0664 permissions and %t1 got 1777. I was having a couple other problems related to lit not removing temp files. Not sure what caused this. but you're right that this is strange, I'd be happy to remove the -p.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

243

A quick grep would suggest there are only tests which use - for stdout and none of these are explicitly testing this. Good idea! I have added a test case here https://reviews.llvm.org/D62817

Also, do you have an opinion on how permissions should be decided when reading from stdin? I think 0664 | executableFile() ? 0111 : 0. So we just default to 0664 and then add execute bits if the header says it is executable.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/same-permissions.test

7

Nit: looks like there's an additional trailing blank line here?

llvm/test/tools/llvm-objcopy/ELF/strip-all-and-remove.test

2

Please do. I don't think your change should cause this behaviour difference. If it does, there's a bug either in your change or in something you've exposed with your change.

[rupprecht]

Thanks for noticing this bug! I think the issue is a little more subtle (I didn't notice it until I really started digging into it), see my last comment especially. I can also include that one in the bug.

llvm/test/tools/llvm-objcopy/ELF/same-permissions.test

4

Another (probably) more portable option is ls -l | cut -f 1 -d ' '. But if you can get stat --format working (and remove the system-linux requirement), that's fine too.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

208–209

We should actually have a check for PreserveDates w/ - as a filename (for input or output) that gives a more useful error (like --preserve-dates requires a file):

$ llvm-objcopy -p /tmp/foo.o /tmp/bar.o
$ llvm-objcopy -p - /tmp/bar.o < /tmp/foo.o
llvm-objcopy: error: '-': No such file or directory

Doesn't have to be this patch tho.

Side note: GNU objcopy appears to treat - as literal filenames.

239

We should copy the same permissions to the split dwo file too

Instead of creating a separate block below as you now have, I think it makes sense to refactor this a tiny bit, e.g. change restoreDateOnFile to a more general restoreStatOnFile method (that takes a PreserveDates arg)

Actually... see my comment below, I think setting based on an initial stat is not actually the right way to go

243

I think 0664 | executableFile() ? 0111 : 0

I think what we'd want is 0666 for object files, 0777 for executables, but respecting the umask, which I *think* means you either need to figure out whether it's an executable when you create the FileBuffer (and let the OS take care of handling the umask), or manually handle the umask when restoring stat.

example w/ GNU objcopy:

$ umask 027
$ chmod 0777 /tmp/foo.o
$ objcopy /tmp/foo.o /tmp/bar.o
$ stat --format='%A %n' /tmp/foo.o /tmp/bar.o
-rwxrwxrwx /tmp/foo.o
-rwxr-x--- /tmp/bar.o

Even though the input file has full r/w/x permissions, the output has the umask applied

246

This should additionally be wrapped in a file error w/ Config.OutputFilename as the filename

[rupprecht]

Looks like GNU objcopy creates files w/ "w+" (i.e. 0666 w/ umask automatically applied) [1], and then does a chmod w/ umask explicitly applied when making it executable [2]:

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/cache.c;h=a49d191bef560506307227e6a7a85c1ff20470c8;hb=HEAD#l633 (FOPEN_WUB is "w+")
[2] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/opncls.c;h=b8cda411e608d13919639cfca361a1ec2cc6322d;hb=HEAD#l678

[abrachet]

Went with @rupprecht suggestion to make output files 0666 if they are not executable and 0777 if they are. Also slightly reorganizes executeObjcopy's handling of --preserve-dates.

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

222

@rupprecht I was a little confused when you said we should copy the same permissions for the split dwo file. Which permissions were you referring to?

230

I grep'd through lib and include and couldn't find any calls to umask so I don't think LLVM currently has a wrapper around chmod(2). This clearly looks out of place though. Looking for suggestions. I didn't bother with error handling yet because I am assuming this is going to change.

239

@rupprecht, could you take a look to see if this is what you were suggesting?

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

47

Weird place for me to put this include, not really sure why I choose there. Regardless I am expecting this to change.

188

This may not be permanent, I am trying to add ObjectFile::isExecutableObject() here https://reviews.llvm.org/D62838

[rupprecht]

Context isn't available in some of the regions; can you reupload the patch w/ context?

Using arc automatically includes full context, but if you upload patches manually, you should format your patch w/ -U99999 as described here: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/test/tools/llvm-objcopy/ELF/execute-permissions.test

1 ↗

(On Diff #203091)

I believe umask (as a shell command and as a syscall) is only available on linux, so these will need guards to be excluded on windows.

• In lit, you can use # UNSUPPORTED: windows to exclude it
• In the code, you should put the umask calls in a #ifndef _WIN32 block.

2 ↗

(On Diff #203091)

Instead of invoking clang, just use dummy files created by yaml2obj (see other tests in this directory)

4 ↗

(On Diff #203091)

I think this also needs a # REQUIRES: shell, although maybe using ls/stat instead is possible (e.g. pipe ls into FileCheck and expect "rwxrwxrwx"

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/respect-umask.test

15 ↗

(On Diff #203091)

Use cmp instead of diff

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

222

I guess what I was picturing is just having the method restoreStatOnFile that preserves dates and permissions, and have that method be called twice for the regular file and the dwo file.
But, I don't think the split dwo feature is ever used w/ executables, so it might be fine to always assume 0666 permissions for that.

227

Although the earlier check guarantees that -p is never used with - as an output file, I think it makes sense to have this check first in the method, with a brief comment on why it's being skipped.

230

I think this actually looks correct, although you need the umask call (and corresponding headers) guarded to exclude windows.
The right place to put it would be in the support library (same place as sys::fs::setPermissions), and just make it noop for the Windows implementation. IMO since this would be the first call to umask, it's fine to leave it here with a TODO to move it into libSupport when someone else needs it.

According to man umask, the umask syscall can never fail, so there is no error handling needed, although perhaps a comment that it is not threadsafe would be good.

255

Binary input (see BinaryELFBuilder::initFileHeader()) currently hardcodes to ET_REL, so this should stick with the default of false.

270

(from the archive writing portion hidden in the context)
Have you looked at how GNU objcopy handles permissions in archives? The archive code creates a NewArchiveMember struct for each object copied, and has a Perms field which currently defaults to 0644; I imagine we should do the similar logic for that too (i.e. either 0666/0777 depending on the executable bit). You probably don't need to do any umask magic for that; the umask should be handled during archive extraction.

[abrachet]

Changed restoreDateOnFile to a more general restoreStatOnFile which also handles permissions. New archive members also get their permissions set based on isExecutableFile()

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

196

Removing the space between (void) and ::umask was clang-formats doing, but I figured I would keep it and ask here. I couldn't find references to calling functions in the global namespace, or unused results in the style guide.

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

195

We could have -p preserve permissions ignoring both the umask and whether or not it is an executable object. Although it is a little bit strange to have --preserve-date do this too, but I figured I would suggest while we are working on this now.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/invalid-preserve-dates.test

1 ↗

(On Diff #203286)

This test (and the corresponding behaviour in the code) feels like it belongs in a separate patch to me.

[rupprecht]

llvm/test/tools/llvm-objcopy/ELF/execute-permissions.test

13–14 ↗

(On Diff #203286)

The copy is unnecessary, just call llvm-objcopy %p/Inputs/alloc-symtab.o %t1

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

196

Usual style for unused variables/return values is void w/ no space, so this looks correct.

229–233

... I should have realized this earlier, but maybe we don't need the umask calls at all if we can just refactor some things:

• If you add some plumbing for the FileBuffer permissions, i.e. make FileBuffer::FileBuffer() take unsigned Flags as a constructor param (0 as a default), then
• Within FileBuffer, create the FileOutputBuffer w/ permissions passed in to the constructor instead of always passing FileOutputBuffer::F_executable, then
• Reorder these calls like thus:

object::Binary &Binary = *BinaryOrErr.get().getBinary();
Exec = isExecutableObject(Binary);
FileBuffer FB(Config.OutputFilename, Exec ? FileOutputBuffer::F_executable : 0);
if (Error E = executeObjcopyOnBinary(Config, Binary, FB))
  return E;

Then I think the permissions should be fine (FileOutputBuffer happens to use 0666/0777 based on that one bit, which matches what we want), including handling umask (handled by the OS when creating files)

[abrachet]

Removes checks that both input and output files are not stdin and stdout when --preserve-date (to be added in a seperate patch). Significantly simplifies the changing of permissions, now done on opening the file which will also respect the umask.

[rupprecht]

Just a couple nits, but I think this is ready to commit now. It can be updated whenever D62838 lands. Thanks for all the iterations!

llvm/tools/llvm-objcopy/Buffer.h

50 ↗

(On Diff #203407)

"explicit" can be removed now that this is two-args (and there's no implicit-conversion risk)

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

152

Can you add a TODO to use isExecutableObject from D62838 once that's submitted?

154

getEType() == ELF::ET_EXEC

[abrachet]

Added a TODO and slight change to isExecutableObject

[abrachet]

llvm/tools/llvm-objcopy/Buffer.h

50 ↗

(On Diff #203407)

I quickly checked if this was still the case because of the default arg, explicit does still protect here. Can be seen here if curious https://godbolt.org/z/nAlqDq

[rupprecht]

llvm/tools/llvm-objcopy/Buffer.h

50 ↗

(On Diff #203407)

Yep! Of course, I forgot about default args affecting that.

[MaskRay]

I believe GNU objcopy has the following logic:

  if (i_ehdrp->e_type == ET_EXEC)
    abfd->flags |= EXEC_P;
  else if (i_ehdrp->e_type == ET_DYN)
    abfd->flags |= DYNAMIC;

// bfd/opncls.c
static inline void
_maybe_make_executable (bfd * abfd)
{
  /* If the file was open for writing and is now executable,
     make it so.  */
  if (abfd->direction == write_direction
      && (abfd->flags & (EXEC_P | DYNAMIC)) != 0)
    {
      struct stat buf;

      if (stat (abfd->filename, &buf) == 0
	  /* Do not attempt to change non-regular files.  This is
	     here especially for configure scripts and kernel builds
	     which run tests with "ld [...] -o /dev/null".  */
	  && S_ISREG(buf.st_mode))
	{
	  unsigned int mask = umask (0);

	  umask (mask);
	  chmod (abfd->filename,
		 (0777
		  & (buf.st_mode | ((S_IXUSR | S_IXGRP | S_IXOTH) &~ mask))));
	}
    }
}

I would not suggest copying this behavior (if ET_EXEC or ET_DYN, chmod +x).
Instead, mirroring the input file's permissions is much more reasonable and gives the least surprise.

[abrachet]

Instead, mirroring the input file's permissions is much more reasonable and gives the least surprise.

@MaskRay, do you think that it should directly mirror the input file's permissions or should it do it as close as possible while respecting the umask?

[MaskRay]

We have 3 choices.

1. cp -p semantics: perm(output) = perm(input)
2. cp semantics: perm(output) = perm(input) & ~umask
3. GNU objcopy (also linker behavior): perm(output) = (ET_EXEC || ET_DYN ? 0777 : 0666) & ~umask

Respecting umask is nice, so I'll go with 2) or 3).

The argument for 2) is: if the input shared object does not have executable bit,
after llvm-objcopy, we do not have to fix up the permission bits.

I don't hate 3) as much as I did before, but still feel we can't do it cleanly. If it is a block/character special file, you probably don't want to change its permission (think: root does llvm-objcopy something /dev/null).

[abrachet]

I think I agree that 2 is our best option here. I think perhaps overlooked is when reading from stdin we don't know what permissions to assign. What I am working on right now just assigns 0775 (the current default for all output files) I don't know if you would think this is worth checking if the object file is marked as executable or not.

If it is a block/character special file, you probably don't want to change its permission (think: root does llvm-objcopy something /dev/null).

I'm not sure I understand why this is specifically a problem for 3. Perhaps sys::fs::setPermissions (and other routines in sys::fs:: which modify set permission bits) should be changed to be weary about the file's type, but I don't understand why this is specifically a problem for 3. Seems like a problem for every option including how llvm-objcopy currently does this.

[MaskRay]

In D62718#1551450, @abrachet wrote:

I think I agree that 2 is our best option here. I think perhaps overlooked is when reading from stdin we don't know what permissions to assign. What I am working on right now just assigns 0775 (the current default for all output files) I don't know if you would think this is worth checking if the object file is marked as executable or not.

If it is a block/character special file, you probably don't want to change its permission (think: root does llvm-objcopy something /dev/null).

I'm not sure I understand why this is specifically a problem for 3. Perhaps sys::fs::setPermissions (and other routines in sys::fs:: which modify set permission bits) should be changed to be weary about the file's type, but I don't understand why this is specifically a problem for 3. Seems like a problem for every option including how llvm-objcopy currently does this.

Sorry, the issue about block/character special file is generic, not specific to 3). Let's see what other reviewers think.

[abrachet]

Output files now mirror input file permissions while respecting the umask.

[abrachet]

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions.test

12 ↗

(On Diff #205745)

I know this won't be allowed, it was just to ask for help on how to do this. I don't want to make the entire test unsupported on Windows just because umask needs to be 0 on systems which have such a thing.

[jhenderson]

I think this looks pretty good, but I'd like D63583 to land first if possible.

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions.test

1 ↗

(On Diff #205745)

Add a comment at the top of this test that explains what the aim of the test is.

12 ↗

(On Diff #205745)

I agree that this probably isn't the right way of doing things, and checking the permissions on Windows is important. How about having two tests, one for Windows and one for Linux? Effectively you already have that, just add REQUIRES: windows to this test and delete this block.

[abrachet]

Split tests into Windows and Unix versions.

[abrachet]

If it is a block/character special file, you probably don't want to change its permission (think: root does llvm-objcopy something /dev/null).

I think we have 3 ways of addressing this:

1. Add flags to various file handling routines in sys::fs to be weary to not change permissions of a special file.
2. Ignore this because it is very unlikely.
3. Not changing permissions of the output file if it is being written over.

I had actually started on 1, but think that it was not worth it, it just clutters their parameters for very little reason. Ultimately I think the problem here is very unlikely for this and other llvm tools. I don't remember the last time I ran anything as su, nor do I think any setuid program would have any reason run llvm-objcopy or any other llvm tools. I think 3 is the obvious compromise, checking if we are writing to a special file is too niche to worry about in my opinion, but not changing permission bits of an existing file would solve the concern raised in a straight forward manner. Personally I am the biggest fan of 2 for the reasons above, I'm not sure that the benefits of 3 outweigh the cost of adding what would be confusing behavior in my opinion.

[MaskRay]

In D62718#1553139, @abrachet wrote:

If it is a block/character special file, you probably don't want to change its permission (think: root does llvm-objcopy something /dev/null).

I think we have 3 ways of addressing this:

1. Add flags to various file handling routines in sys::fs to be weary to not change permissions of a special file.
2. Ignore this because it is very unlikely.
3. Not changing permissions of the output file if it is being written over.

I had actually started on 1, but think that it was not worth it, it just clutters their parameters for very little reason. Ultimately I think the problem here is very unlikely for this and other llvm tools. I don't remember the last time I ran anything as su, nor do I think any setuid program would have any reason run llvm-objcopy or any other llvm tools. I think 3 is the obvious compromise, checking if we are writing to a special file is too niche to worry about in my opinion, but not changing permission bits of an existing file would solve the concern raised in a straight forward manner. Personally I am the biggest fan of 2 for the reasons above, I'm not sure that the benefits of 3 outweigh the cost of adding what would be confusing behavior in my opinion.

2 should be fine..

[jhenderson]

LGTM, with the suggested comment changes, but please wait for the other patch, and @MaskRay's agreement.

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test

4 ↗

(On Diff #205945)

You could probably combine this comment paragraph with the initial one.

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-win.test

4 ↗

(On Diff #205945)

This should have a comment explaining why this test is Windows-specific.

[rupprecht]

(ditto that Ray should take another look, but looks mostly fine to me)

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

200–205

Once you land D63583, you should be able to make Stat a const parameter again, since setPermissions will be taking care of masking Stat.permissions().

211

0777

(the umask should be what masks other/world permissions)

242

0666

[abrachet]

Changed default permissions when reading from stdin.

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

211

Great, thanks!

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test

4 ↗

(On Diff #206107)

"The Unix version..."

Why might the tests fail? Be a bit more explicit (specifically, they might fail because the user has a different umask to the build bots).

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-win.test

4 ↗

(On Diff #205945)

Still missing this comment... It'll largely be the same as the Linux one, just inverted.

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions.test

1 ↗

(On Diff #206107)

I think you accidentally re-added this file?

[abrachet]

Added comments to tests.

[abrachet]

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test

4 ↗

(On Diff #205945)

You could probably combine this comment paragraph with the initial one.

I separated these because the initial one is explaining the entire test but the second comment is explaining the # UNSUPPORTED: system-windows. Do you still think I should combine them, I don't have a large preference.

[jhenderson]

LGTM. @MaskRay should approve before this gets committed though.

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test

4 ↗

(On Diff #205945)

This is fine now. The extra detail makes it a more meaningful independent paragraph.

[phosek]

This broke tests:

FAIL: LLVM :: tools/llvm-objcopy/ELF/respect-umask.test (31512 of 32250)
******************** TEST 'LLVM :: tools/llvm-objcopy/ELF/respect-umask.test' FAILED ********************
Script:
--
: 'RUN: at line 8';   touch /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 9';   chmod 0755 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 10';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0755
: 'RUN: at line 11';   chmod 0500 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 12';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0500
: 'RUN: at line 13';   chmod 0555 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 14';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0555
: 'RUN: at line 16';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/yaml2obj /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test -o /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 18';   umask 0022
: 'RUN: at line 19';   chmod 0777 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 20';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/llvm-objcopy /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1
: 'RUN: at line 21';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1 | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms
: 'RUN: at line 22';   cmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0755
: 'RUN: at line 24';   umask 0237
: 'RUN: at line 25';   chmod 0707 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 26';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/llvm-objcopy /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1
: 'RUN: at line 27';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1 | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms
: 'RUN: at line 28';   cmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0500
: 'RUN: at line 30';   umask 0222
: 'RUN: at line 31';   chmod 0777 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 32';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/llvm-objcopy /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1
: 'RUN: at line 33';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1 | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms
: 'RUN: at line 34';   cmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0555
--
Exit Code: 1

Command Output (stderr):
--
yaml2obj: Error opening '/b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp': Permission denied

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 105.45s
********************
Failing Tests (1):
    LLVM :: tools/llvm-objcopy/ELF/respect-umask.test

  Expected Passes    : 23888
  Expected Failures  : 63
  Unsupported Tests  : 8298
  Unexpected Failures: 1
[557/559] Linking CXX executable tools/clang/unittests/Tooling/ToolingTests
FAILED: test/CMakeFiles/check-llvm

Even after the fix r365170 the tests are still failing.

[MaskRay]

In D62718#1570995, @phosek wrote:

This broke tests:

FAIL: LLVM :: tools/llvm-objcopy/ELF/respect-umask.test (31512 of 32250)
******************** TEST 'LLVM :: tools/llvm-objcopy/ELF/respect-umask.test' FAILED ********************
Script:
--
: 'RUN: at line 8';   touch /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 9';   chmod 0755 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 10';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0755
: 'RUN: at line 11';   chmod 0500 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 12';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0500
: 'RUN: at line 13';   chmod 0555 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 14';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0555
: 'RUN: at line 16';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/yaml2obj /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test -o /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 18';   umask 0022
: 'RUN: at line 19';   chmod 0777 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 20';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/llvm-objcopy /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1
: 'RUN: at line 21';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1 | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms
: 'RUN: at line 22';   cmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0755
: 'RUN: at line 24';   umask 0237
: 'RUN: at line 25';   chmod 0707 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 26';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/llvm-objcopy /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1
: 'RUN: at line 27';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1 | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms
: 'RUN: at line 28';   cmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0500
: 'RUN: at line 30';   umask 0222
: 'RUN: at line 31';   chmod 0777 /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp
: 'RUN: at line 32';   /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/bin/llvm-objcopy /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1
: 'RUN: at line 33';   ls -l /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1 | cut -f 1 -d ' ' > /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms
: 'RUN: at line 34';   cmp /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp1.perms /b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp.0555
--
Exit Code: 1

Command Output (stderr):
--
yaml2obj: Error opening '/b/s/w/ir/k/recipe_cleanup/clangz_eJan/llvm_build_dir/test/tools/llvm-objcopy/ELF/Output/respect-umask.test.tmp': Permission denied

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 105.45s
********************
Failing Tests (1):
    LLVM :: tools/llvm-objcopy/ELF/respect-umask.test

  Expected Passes    : 23888
  Expected Failures  : 63
  Unsupported Tests  : 8298
  Unexpected Failures: 1
[557/559] Linking CXX executable tools/clang/unittests/Tooling/ToolingTests
FAILED: test/CMakeFiles/check-llvm

Even after the fix r365170 the tests are still failing.

Fixed by rL365172.

The umask 02* tests do not work, so I deleted them for now.

[abrachet]

Thanks you @phosek and @MaskRay. I really don't know how these worked on my machine, I'm never in su! Sorry about that.

llvm/test/tools/llvm-objcopy/ELF/same-permissions.test

+# REQUIRES: system-linux
+# RUN: llvm-objcopy %p/Inputs/alloc-symtab.o %t
+# RUN: stat --printf=%a %p/Inputs/alloc-symtab.o > %t1
+# RUN: stat --printf=%a %t > %t2
+# RUN: diff %t1 %t2
+

llvm/test/tools/llvm-objcopy/ELF/strip-all-and-remove.test

 # RUN: yaml2obj %s > %t
-# RUN: cp %t %t1
+# RUN: cp -p %t %t1
 
 # RUN: llvm-strip --remove-section=.text.bar %t
 # RUN: llvm-readobj --file-headers --sections --symbols %t | FileCheck %s
 
 # RUN: llvm-objcopy --strip-all --remove-section=.text.bar %t1 %t1
 # RUN: cmp %t %t1
 
 !ELF

llvm/tools/llvm-objcopy/Buffer.cpp

Error FileBuffer::allocate(size_t Size) {
   // creation/truncation until commit() to avoid side effects if something
   // happens between allocate() and commit().
   if (Size == 0) {
     EmptyFile = true;
     return Error::success();
   }
 
   Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-      FileOutputBuffer::create(getName(), Size, FileOutputBuffer::F_executable);
+      FileOutputBuffer::create(getName(), Size);
   // FileOutputBuffer::create() returns an Error that is just a wrapper around
   // std::error_code. Wrap it in FileError to include the actual filename.
   if (!BufferOrErr)
     return createFileError(getName(), BufferOrErr.takeError());
   Buf = std::move(*BufferOrErr);
   return Error::success();
 }
 

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <memory>
 #include <string>
 #include <system_error>
 #include <utility>
 
 namespace llvm {
 namespace objcopy {
 
 // The name this program was invoked as.
 StringRef ToolName;
 
 LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In))
     return coff::executeObjcopyOnBinary(Config, *COFFBinary, Out);
   else if (auto *MachOBinary = dyn_cast<object::MachOObjectFile>(&In))
     return macho::executeObjcopyOnBinary(Config, *MachOBinary, Out);
   else
     return createStringError(object_error::invalid_file_type,
                              "Unsupported object file format");
 }
 
 static Error executeObjcopyOnArchive(const CopyConfig &Config,
                                      const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {
     Expected<StringRef> ChildNameOrErr = Child.getName();
     if (!ChildNameOrErr)
       return createFileError(Ar.getFileName(), ChildNameOrErr.takeError());
 
     Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
     if (!ChildOrErr)
static Error executeObjcopyOnArchive(const CopyConfig &Config,
 
   return deepWriteArchive(Config.OutputFilename, NewArchiveMembers,
                           Ar.hasSymbolTable(), Ar.kind(),
                           Config.DeterministicArchives, Ar.isThin());
 }
 
 static Error restoreDateOnFile(StringRef Filename,
                                const sys::fs::file_status &Stat) {
   int FD;
 
   if (auto EC =
           sys::fs::openFileForWrite(Filename, FD, sys::fs::CD_OpenExisting))
     return createFileError(Filename, EC);
 
   if (auto EC = sys::fs::setLastAccessAndModificationTime(
           FD, Stat.getLastAccessedTime(), Stat.getLastModificationTime()))
     return createFileError(Filename, EC);
 
   if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(Filename, EC);
 
   return Error::success();
 }
 
 /// The function executeObjcopy does the higher level dispatch based on the type
 /// of input (raw binary, archive or single object file) and takes care of the
 /// format-agnostic modifications, i.e. preserving dates.
 static Error executeObjcopy(const CopyConfig &Config) {
   sys::fs::file_status Stat;
-  if (Config.PreserveDates)
-    if (auto EC = sys::fs::status(Config.InputFilename, Stat))
-      return createFileError(Config.InputFilename, EC);
 
   if (Config.InputFormat == "binary") {
     auto BufOrErr = MemoryBuffer::getFile(Config.InputFilename);
     if (!BufOrErr)
       return createFileError(Config.InputFilename, BufOrErr.getError());
     FileBuffer FB(Config.OutputFilename);
     if (Error E = executeObjcopyOnRawBinary(Config, *BufOrErr->get(), FB))
       return E;
   } else {
     Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(Config.InputFilename);
     if (!BinaryOrErr)
       return createFileError(Config.InputFilename, BinaryOrErr.takeError());
 
     if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
       if (Error E = executeObjcopyOnArchive(Config, *Ar))
         return E;
     } else {
       FileBuffer FB(Config.OutputFilename);
       if (Error E = executeObjcopyOnBinary(Config,
                                            *BinaryOrErr.get().getBinary(), FB))
         return E;
     }
   }
 
   if (Config.PreserveDates) {
     if (Error E = restoreDateOnFile(Config.OutputFilename, Stat))
       return E;
     if (!Config.SplitDWO.empty())
       if (Error E = restoreDateOnFile(Config.SplitDWO, Stat))
         return E;
   }
+  if (Config.OutputFilename != "-")
+    if (std::error_code E = 
+      sys::fs::setPermissions(Config.OutputFilename, Stat.permissions()))
+      return errorCodeToError(E);
 
   return Error::success();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
   bool IsStrip = sys::path::stem(ToolName).contains("strip");
   Expected<DriverConfig> DriverConfig =
       IsStrip ? parseStripOptions(makeArrayRef(argv + 1, argc))
               : parseObjcopyOptions(makeArrayRef(argv + 1, argc));
   if (!DriverConfig) {
     logAllUnhandledErrors(DriverConfig.takeError(),
                           WithColor::error(errs(), ToolName));
     return 1;
   }
   for (const CopyConfig &CopyConfig : DriverConfig->CopyConfigs) {
     if (Error E = executeObjcopy(CopyConfig)) {
       logAllUnhandledErrors(std::move(E), WithColor::error(errs(), ToolName));
       return 1;
     }
   }
 
   return 0;
 }