[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1233e2e66831: [Support] Dont set all_exe 
mode by default for file written by llvm… (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

Files:
  llvm/lib/Support/raw_ostream.cpp
  llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
  llvm/unittests/Support/raw_ostream_test.cpp

Index: llvm/unittests/Support/raw_ostream_test.cpp
===
--- llvm/unittests/Support/raw_ostream_test.cpp
+++ llvm/unittests/Support/raw_ostream_test.cpp
@@ -504,6 +504,31 @@
   checkFileData(Path, "HelloWorld");
 }
 
+#ifndef _WIN32
+TEST(raw_ostreamTest, filePermissions) {
+  // Set umask to be permissive of all permissions.
+  unsigned OldMask = ::umask(0);
+
+  llvm::unittest::TempDir RootTestDirectory("writToOutput", /*Unique*/ true);
+  SmallString<128> Path(RootTestDirectory.path());
+  sys::path::append(Path, "test.txt");
+
+  ASSERT_THAT_ERROR(writeToOutput(Path,
+  [](raw_ostream ) -> Error {
+Out << "HelloWorld";
+return Error::success();
+  }),
+Succeeded());
+
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(Perms) << "should be able to get permissions";
+  // Verify the permission bits set by writeToOutput are read and write only.
+  EXPECT_EQ(Perms.get(), llvm::sys::fs::all_read | llvm::sys::fs::all_write);
+
+  ::umask(OldMask);
+}
+#endif
+
 TEST(raw_ostreamTest, writeToNonexistingPath) {
   StringRef FileName = "/_bad/_path";
   std::string ErrorMessage = toString(createFileError(
Index: llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
===
--- /dev/null
+++ llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
@@ -0,0 +1,51 @@
+## The Unix version of this test must use umask(1) because
+## llvm-dwarfutil respects the umask in setting output permissions.
+## Setting the umask to 0 ensures deterministic permissions across
+## test environments.
+# UNSUPPORTED: system-windows
+# REQUIRES: shell
+
+# RUN: touch %t
+# RUN: chmod 0777 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0777
+# RUN: chmod 0666 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0666
+# RUN: chmod 0640 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0640
+
+## Set umask to be permissive of all permissions,
+## only test mirroring of permissions.
+# RUN: umask 0
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: chmod 0777 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0777
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0777
+
+# RUN: chmod 0666 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0666
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0666
+
+# RUN: chmod 0640 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0640
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0640
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
Index: llvm/lib/Support/raw_ostream.cpp
===
--- llvm/lib/Support/raw_ostream.cpp
+++ llvm/lib/Support/raw_ostream.cpp
@@ -1007,7 +1007,7 @@
 return Write(Out);
   }
 
-  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
+  unsigned Mode = sys::fs::all_read | sys::fs::all_write;
   Expected Temp =
   sys::fs::TempFile::create(OutputFileName + ".temp-stream-%%", Mode);
   if (!Temp)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D153652#4459489 , @jhenderson 
wrote:

> The new test LGTM, albeit with one query: I assume `umask` sets some global 
> state. When the lit unit tests are running, do the tests within the same 
> process run sequentially, or are they in parallel at all? If the latter, 
> there could be some thread-safety issue, although I suspect the tests are run 
> sequentially.

Good question. For each lit test, `llvm-lit` will run it on a separate worker 
process 
, 
so we should be safe here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

The new test LGTM, albeit with one query: I assume `umask` sets some global 
state. When the lit unit tests are running, do the tests within the same 
process run sequentially, or are they in parallel at all? If the latter, there 
could be some thread-safety issue, although I suspect the tests are run 
sequentially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: llvm/unittests/Support/raw_ostream_test.cpp:525
+  ASSERT_TRUE(Perms) << "should be able to get permissions";
+  // Verify that writeToOutput doesn't set exe bit.
+  EXPECT_EQ(Perms.get(), llvm::sys::fs::all_read | llvm::sys::fs::all_write);

avl wrote:
> nit: comment looks a bit inconsistent as we check for read bits.
refined the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 535712.
hokein marked an inline comment as done.
hokein added a comment.

address the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

Files:
  llvm/lib/Support/raw_ostream.cpp
  llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
  llvm/unittests/Support/raw_ostream_test.cpp

Index: llvm/unittests/Support/raw_ostream_test.cpp
===
--- llvm/unittests/Support/raw_ostream_test.cpp
+++ llvm/unittests/Support/raw_ostream_test.cpp
@@ -504,6 +504,31 @@
   checkFileData(Path, "HelloWorld");
 }
 
+#ifndef _WIN32
+TEST(raw_ostreamTest, filePermissions) {
+  // Set umask to be permissive of all permissions.
+  unsigned OldMask = ::umask(0);
+
+  llvm::unittest::TempDir RootTestDirectory("writToOutput", /*Unique*/ true);
+  SmallString<128> Path(RootTestDirectory.path());
+  sys::path::append(Path, "test.txt");
+
+  ASSERT_THAT_ERROR(writeToOutput(Path,
+  [](raw_ostream ) -> Error {
+Out << "HelloWorld";
+return Error::success();
+  }),
+Succeeded());
+
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(Perms) << "should be able to get permissions";
+  // Verify the permission bits set by writeToOutput are read and write only.
+  EXPECT_EQ(Perms.get(), llvm::sys::fs::all_read | llvm::sys::fs::all_write);
+
+  ::umask(OldMask);
+}
+#endif
+
 TEST(raw_ostreamTest, writeToNonexistingPath) {
   StringRef FileName = "/_bad/_path";
   std::string ErrorMessage = toString(createFileError(
Index: llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
===
--- /dev/null
+++ llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
@@ -0,0 +1,51 @@
+## The Unix version of this test must use umask(1) because
+## llvm-dwarfutil respects the umask in setting output permissions.
+## Setting the umask to 0 ensures deterministic permissions across
+## test environments.
+# UNSUPPORTED: system-windows
+# REQUIRES: shell
+
+# RUN: touch %t
+# RUN: chmod 0777 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0777
+# RUN: chmod 0666 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0666
+# RUN: chmod 0640 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0640
+
+## Set umask to be permissive of all permissions,
+## only test mirroring of permissions.
+# RUN: umask 0
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: chmod 0777 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0777
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0777
+
+# RUN: chmod 0666 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0666
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0666
+
+# RUN: chmod 0640 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0640
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0640
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
Index: llvm/lib/Support/raw_ostream.cpp
===
--- llvm/lib/Support/raw_ostream.cpp
+++ llvm/lib/Support/raw_ostream.cpp
@@ -1007,7 +1007,7 @@
 return Write(Out);
   }
 
-  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
+  unsigned Mode = sys::fs::all_read | sys::fs::all_write;
   Expected Temp =
   sys::fs::TempFile::create(OutputFileName + ".temp-stream-%%", Mode);
   if (!Temp)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Alexey Lapshin via Phabricator via cfe-commits]

avl added a comment.

this LGTM. please, wait if James has any concern.




Comment at: llvm/unittests/Support/raw_ostream_test.cpp:525
+  ASSERT_TRUE(Perms) << "should be able to get permissions";
+  // Verify that writeToOutput doesn't set exe bit.
+  EXPECT_EQ(Perms.get(), llvm::sys::fs::all_read | llvm::sys::fs::all_write);

nit: comment looks a bit inconsistent as we check for read bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+

jhenderson wrote:
> hokein wrote:
> > jhenderson wrote:
> > > Here and below, rather than just checking the all_exe bit, let's check 
> > > the permissions are exactly what are expected (e.g. does it have the 
> > > read/write perms?). 
> > checking all existing bits is a bit tricky here (I tried it, then gave up):
> > 
> > - createTemporaryFile() creates a file with `owner_read | owner_write`
> > - writeToOutput() sets the written file to `all_read | all_write`
> > 
> > Both API don't provide a way to customize these bits, and they're internal 
> > details. We could test against them, but testing the implementation details 
> > seems subtle. And here we aim to verify the exe-bit not set by the 
> > `writeToOutput`, so I think just testing the exe-bit is not set should be 
> > enough. 
> This argument doesn't make much sense to me. Why are the `all_read` and 
> `all_write` bits implementation details that shouldn't be tested when the 
> lack of `all_exe` is?
> 
> This test is for testing `writeToOutput`. Part of `writeToOutput`'s behaviour 
> appears to be to create a file with the `all_read` and `all_write` bits set. 
> Therefore, we should be testing that behaviour. As there was already one 
> issue with the permission bits of the file this method creates, and you are 
> directly modifiyng a test to add permissions testing, I think it's justified 
> to request testing of the other bits.
OK, I changed the test to verify the all_read and all_write bits, please take 
another look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 535309.
hokein added a comment.

compare the file-permission bits in unittest


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

Files:
  llvm/lib/Support/raw_ostream.cpp
  llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
  llvm/unittests/Support/raw_ostream_test.cpp

Index: llvm/unittests/Support/raw_ostream_test.cpp
===
--- llvm/unittests/Support/raw_ostream_test.cpp
+++ llvm/unittests/Support/raw_ostream_test.cpp
@@ -504,6 +504,31 @@
   checkFileData(Path, "HelloWorld");
 }
 
+#ifndef _WIN32
+TEST(raw_ostreamTest, filePermissions) {
+  // Set umask to be permissive of all permissions.
+  unsigned OldMask = ::umask(0);
+
+  llvm::unittest::TempDir RootTestDirectory("writToOutput", /*Unique*/ true);
+  SmallString<128> Path(RootTestDirectory.path());
+  sys::path::append(Path, "test.txt");
+
+  ASSERT_THAT_ERROR(writeToOutput(Path,
+  [](raw_ostream ) -> Error {
+Out << "HelloWorld";
+return Error::success();
+  }),
+Succeeded());
+
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(Perms) << "should be able to get permissions";
+  // Verify that writeToOutput doesn't set exe bit.
+  EXPECT_EQ(Perms.get(), llvm::sys::fs::all_read | llvm::sys::fs::all_write);
+
+  ::umask(OldMask);
+}
+#endif
+
 TEST(raw_ostreamTest, writeToNonexistingPath) {
   StringRef FileName = "/_bad/_path";
   std::string ErrorMessage = toString(createFileError(
Index: llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
===
--- /dev/null
+++ llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
@@ -0,0 +1,51 @@
+## The Unix version of this test must use umask(1) because
+## llvm-dwarfutil respects the umask in setting output permissions.
+## Setting the umask to 0 ensures deterministic permissions across
+## test environments.
+# UNSUPPORTED: system-windows
+# REQUIRES: shell
+
+# RUN: touch %t
+# RUN: chmod 0777 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0777
+# RUN: chmod 0666 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0666
+# RUN: chmod 0640 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0640
+
+## Set umask to be permissive of all permissions,
+## only test mirroring of permissions.
+# RUN: umask 0
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: chmod 0777 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0777
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0777
+
+# RUN: chmod 0666 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0666
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0666
+
+# RUN: chmod 0640 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0640
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0640
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
Index: llvm/lib/Support/raw_ostream.cpp
===
--- llvm/lib/Support/raw_ostream.cpp
+++ llvm/lib/Support/raw_ostream.cpp
@@ -1007,7 +1007,7 @@
 return Write(Out);
   }
 
-  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
+  unsigned Mode = sys::fs::all_read | sys::fs::all_write;
   Expected Temp =
   sys::fs::TempFile::create(OutputFileName + ".temp-stream-%%", Mode);
   if (!Temp)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+

hokein wrote:
> jhenderson wrote:
> > Here and below, rather than just checking the all_exe bit, let's check the 
> > permissions are exactly what are expected (e.g. does it have the read/write 
> > perms?). 
> checking all existing bits is a bit tricky here (I tried it, then gave up):
> 
> - createTemporaryFile() creates a file with `owner_read | owner_write`
> - writeToOutput() sets the written file to `all_read | all_write`
> 
> Both API don't provide a way to customize these bits, and they're internal 
> details. We could test against them, but testing the implementation details 
> seems subtle. And here we aim to verify the exe-bit not set by the 
> `writeToOutput`, so I think just testing the exe-bit is not set should be 
> enough. 
This argument doesn't make much sense to me. Why are the `all_read` and 
`all_write` bits implementation details that shouldn't be tested when the lack 
of `all_exe` is?

This test is for testing `writeToOutput`. Part of `writeToOutput`'s behaviour 
appears to be to create a file with the `all_read` and `all_write` bits set. 
Therefore, we should be testing that behaviour. As there was already one issue 
with the permission bits of the file this method creates, and you are directly 
modifiyng a test to add permissions testing, I think it's justified to request 
testing of the other bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D153652#4451292 , @jhenderson 
wrote:

> The updated unit test is failing on Windows in the pre-merge checks. Please 
> investigate and fix as appropriate.

Good catch, thanks. I have restricted the unittest to linux only, I think it 
should be fine -- the behavior on windows is a bit different, the exe bit is 
set even you only set `read|write` bits, the `unittests/Support/Path.cpp` 
verifies that behavior.




Comment at: llvm/unittests/Support/raw_ostream_test.cpp:499
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);

avl wrote:
> !!  looks a bit unclear. Probably check it in more explicit way?
> 
> EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe)); 
sure, done.



Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+

jhenderson wrote:
> Here and below, rather than just checking the all_exe bit, let's check the 
> permissions are exactly what are expected (e.g. does it have the read/write 
> perms?). 
checking all existing bits is a bit tricky here (I tried it, then gave up):

- createTemporaryFile() creates a file with `owner_read | owner_write`
- writeToOutput() sets the written file to `all_read | all_write`

Both API don't provide a way to customize these bits, and they're internal 
details. We could test against them, but testing the implementation details 
seems subtle. And here we aim to verify the exe-bit not set by the 
`writeToOutput`, so I think just testing the exe-bit is not set should be 
enough. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 534917.
hokein marked 2 inline comments as done.
hokein added a comment.

fix the test failures on windows


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

Files:
  llvm/lib/Support/raw_ostream.cpp
  llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
  llvm/unittests/Support/raw_ostream_test.cpp


Index: llvm/unittests/Support/raw_ostream_test.cpp
===
--- llvm/unittests/Support/raw_ostream_test.cpp
+++ llvm/unittests/Support/raw_ostream_test.cpp
@@ -495,6 +495,11 @@
   ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, Path));
   FileRemover Cleanup(Path);
 
+#ifndef _WIN32
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe));
+#endif
+
   ASSERT_THAT_ERROR(writeToOutput(Path,
   [](raw_ostream ) -> Error {
 Out << "HelloWorld";
@@ -502,6 +507,12 @@
   }),
 Succeeded());
   checkFileData(Path, "HelloWorld");
+
+#ifndef _WIN32
+  // No exe bit set by the writeToOutput API.
+  Perms = llvm::sys::fs::getPermissions(Path);
+  EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe));
+#endif
 }
 
 TEST(raw_ostreamTest, writeToNonexistingPath) {
Index: llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
===
--- /dev/null
+++ llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
@@ -0,0 +1,51 @@
+## The Unix version of this test must use umask(1) because
+## llvm-dwarfutil respects the umask in setting output permissions.
+## Setting the umask to 0 ensures deterministic permissions across
+## test environments.
+# UNSUPPORTED: system-windows
+# REQUIRES: shell
+
+# RUN: touch %t
+# RUN: chmod 0777 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0777
+# RUN: chmod 0666 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0666
+# RUN: chmod 0640 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0640
+
+## Set umask to be permissive of all permissions,
+## only test mirroring of permissions.
+# RUN: umask 0
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: chmod 0777 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0777
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0777
+
+# RUN: chmod 0666 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0666
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0666
+
+# RUN: chmod 0640 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0640
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0640
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
Index: llvm/lib/Support/raw_ostream.cpp
===
--- llvm/lib/Support/raw_ostream.cpp
+++ llvm/lib/Support/raw_ostream.cpp
@@ -1007,7 +1007,7 @@
 return Write(Out);
   }
 
-  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
+  unsigned Mode = sys::fs::all_read | sys::fs::all_write;
   Expected Temp =
   sys::fs::TempFile::create(OutputFileName + ".temp-stream-%%", Mode);
   if (!Temp)


Index: llvm/unittests/Support/raw_ostream_test.cpp
===
--- llvm/unittests/Support/raw_ostream_test.cpp
+++ llvm/unittests/Support/raw_ostream_test.cpp
@@ -495,6 +495,11 @@
   ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, Path));
   FileRemover Cleanup(Path);
 
+#ifndef _WIN32
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe));
+#endif
+
   ASSERT_THAT_ERROR(writeToOutput(Path,
   [](raw_ostream ) -> Error {
 Out << "HelloWorld";
@@ -502,6 +507,12 @@
   }),
 Succeeded());
   checkFileData(Path, "HelloWorld");
+
+#ifndef _WIN32
+  // No exe bit set by the writeToOutput API.
+  Perms = llvm::sys::fs::getPermissions(Path);
+  EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe));
+#endif
 }
 
 TEST(raw_ostreamTest, writeToNonexistingPath) {
Index: llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

The updated unit test is failing on Windows in the pre-merge checks. Please 
investigate and fix as appropriate.




Comment at: llvm/unittests/Support/raw_ostream_test.cpp:500
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+

Here and below, rather than just checking the all_exe bit, let's check the 
permissions are exactly what are expected (e.g. does it have the read/write 
perms?). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Alexey Lapshin via Phabricator via cfe-commits]

avl added inline comments.



Comment at: 
llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test:2
+## The Unix version of this test must use umask(1) because
+## llvm-darfutil respects the umask in setting output permissions.
+## Setting the umask to 0 ensures deterministic permissions across





Comment at: llvm/unittests/Support/raw_ostream_test.cpp:499
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);

!!  looks a bit unclear. Probably check it in more explicit way?

EXPECT_TRUE(Perms && !(*Perms & llvm::sys::fs::all_exe)); 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comment.

> unit test which checks llvm::writeOutput may be added to 
> raw_ostream_test.cpp. Let`s this test check the case when "all_exec" is not 
> set. i.e. that after default usage of llvm::writeOutput the resulting file 
> does not have "all_exec", which is exact purpose of this patch.

OK, added one.




Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1
+# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o
+# RUN: chmod a+x %t.o

jhenderson wrote:
> hokein wrote:
> > jhenderson wrote:
> > > It might make sense to reuse the test code from llvm-objcopy's 
> > > mirror-permissions tests. See my other inline comment too.
> > by reusing the code, I think you mean calling the `llvm-dwardfutil` tool in 
> > the `llvm-objcopy` lit test, I'm not sure it is a good idea, calling a 
> > different tool in the 
> > `llvm/test/tools/llvm-objcopy/mirror-permissions-*.test` seems like 
> > violating the layer.
> I meant that you could copy and adapt the existing llvm-objcopy test to 
> create a new llvm-dwarfutil test (in the llvm-dwarfutil test folder).
I see, adjusted the test per comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 534507.
hokein added a comment.

address comments:

- add unittest for llvm::writeToOutput API
- refine the llvm-dwarfutil tool lit test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

Files:
  llvm/lib/Support/raw_ostream.cpp
  llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
  llvm/unittests/Support/raw_ostream_test.cpp


Index: llvm/unittests/Support/raw_ostream_test.cpp
===
--- llvm/unittests/Support/raw_ostream_test.cpp
+++ llvm/unittests/Support/raw_ostream_test.cpp
@@ -495,6 +495,10 @@
   ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, Path));
   FileRemover Cleanup(Path);
 
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+
   ASSERT_THAT_ERROR(writeToOutput(Path,
   [](raw_ostream ) -> Error {
 Out << "HelloWorld";
@@ -502,6 +506,10 @@
   }),
 Succeeded());
   checkFileData(Path, "HelloWorld");
+  // No exe bit set by the writeToOutput API.
+  Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
 }
 
 TEST(raw_ostreamTest, writeToNonexistingPath) {
Index: llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
===
--- /dev/null
+++ llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
@@ -0,0 +1,51 @@
+## The Unix version of this test must use umask(1) because
+## llvm-darfutil respects the umask in setting output permissions.
+## Setting the umask to 0 ensures deterministic permissions across
+## test environments.
+# UNSUPPORTED: system-windows
+# REQUIRES: shell
+
+# RUN: touch %t
+# RUN: chmod 0777 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0777
+# RUN: chmod 0666 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0666
+# RUN: chmod 0640 %t
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.0640
+
+## Set umask to be permissive of all permissions,
+## only test mirroring of permissions.
+# RUN: umask 0
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: chmod 0777 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0777
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0777
+
+# RUN: chmod 0666 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0666
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0666
+
+# RUN: chmod 0640 %t
+# RUN: llvm-dwarfutil --no-garbage-collection %t %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0640
+# RUN: llvm-dwarfutil --garbage-collection --separate-debug-file %t %t2
+# RUN: ls -l %t2 | cut -f 1 -d ' ' > %t2.perms
+# RUN: cmp %t2.perms %t.0640
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_X86_64
Index: llvm/lib/Support/raw_ostream.cpp
===
--- llvm/lib/Support/raw_ostream.cpp
+++ llvm/lib/Support/raw_ostream.cpp
@@ -1007,7 +1007,7 @@
 return Write(Out);
   }
 
-  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
+  unsigned Mode = sys::fs::all_read | sys::fs::all_write;
   Expected Temp =
   sys::fs::TempFile::create(OutputFileName + ".temp-stream-%%", Mode);
   if (!Temp)


Index: llvm/unittests/Support/raw_ostream_test.cpp
===
--- llvm/unittests/Support/raw_ostream_test.cpp
+++ llvm/unittests/Support/raw_ostream_test.cpp
@@ -495,6 +495,10 @@
   ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, Path));
   FileRemover Cleanup(Path);
 
+  ErrorOr Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
+
   ASSERT_THAT_ERROR(writeToOutput(Path,
   [](raw_ostream ) -> Error {
 Out << "HelloWorld";
@@ -502,6 +506,10 @@
   }),
 Succeeded());
   checkFileData(Path, "HelloWorld");
+  // No exe bit set by the writeToOutput API.
+  Perms = llvm::sys::fs::getPermissions(Path);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(0, *Perms & llvm::sys::fs::all_exe);
 }
 
 TEST(raw_ostreamTest, writeToNonexistingPath) {
Index: llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
===
--- 

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1
+# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o
+# RUN: chmod a+x %t.o

hokein wrote:
> jhenderson wrote:
> > It might make sense to reuse the test code from llvm-objcopy's 
> > mirror-permissions tests. See my other inline comment too.
> by reusing the code, I think you mean calling the `llvm-dwardfutil` tool in 
> the `llvm-objcopy` lit test, I'm not sure it is a good idea, calling a 
> different tool in the 
> `llvm/test/tools/llvm-objcopy/mirror-permissions-*.test` seems like violating 
> the layer.
I meant that you could copy and adapt the existing llvm-objcopy test to create 
a new llvm-dwarfutil test (in the llvm-dwarfutil test folder).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Alexey Lapshin via Phabricator via cfe-commits]

avl added a comment.

In D153652#4448157 , @hokein wrote:

> Thanks for the comments.
>
> In D153652#4447976 , @jhenderson 
> wrote:
>
>> Is there anything that can be done to use gtest unit tests for this? The two 
>> lit tests are useful, but the problem with them is that if they switch to 
>> using a different approach to emitting their data, the lit tests won't cover 
>> the Support code you're actually changing.
>
> The usages of `llvm::writeOutput` are in the ToolMain.cpp files, I don't see 
> a way to unittest them.

unit test which checks `llvm::writeOutput` may be added to 
raw_ostream_test.cpp. Let`s this test check the case when "all_exec" is not 
set. i.e. that after default usage of `llvm::writeOutput` the resulting file 
does not have "all_exec", which is exact purpose of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153652: [Support] Don't set "all_exe" mode by default for file written by llvm::writeToOutput

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comments.

In D153652#4447976 , @jhenderson 
wrote:

> Is there anything that can be done to use gtest unit tests for this? The two 
> lit tests are useful, but the problem with them is that if they switch to 
> using a different approach to emitting their data, the lit tests won't cover 
> the Support code you're actually changing.

The usages of `llvm::writeOutput` are in the ToolMain.cpp files, I don't see a 
way to unittest them.

> Some commit message nits:
>
>> [llvm][Support] Don'tt set "all_exe" mode by default for file written by 
>> llvm::writeToOutput.
>
> There's no need to include the `[llvm]` tag - the `[Support]` tag already 
> indicates the library clearly enough IMHO. Also, typo "Don'tt" -> "Don't" and 
> we don't usually end commit titles with a ".".

Thanks, updated.

> Does the clang include cleaner tool have testing that covers its usage of 
> this API?

No, the clang-include-cleaner binary tool just literally writes the text code 
to an output file, it unlikes the `llvm-dwardfutil` and `llvm-objcopy` tools 
which has a preserving-input-file-permissions requirement.




Comment at: llvm/test/tools/llvm-dwarfutil/ELF/X86/file-permissions.test:1
+# RUN: yaml2obj %p/Inputs/common.yaml -o %t.o
+# RUN: chmod a+x %t.o

jhenderson wrote:
> It might make sense to reuse the test code from llvm-objcopy's 
> mirror-permissions tests. See my other inline comment too.
by reusing the code, I think you mean calling the `llvm-dwardfutil` tool in the 
`llvm-objcopy` lit test, I'm not sure it is a good idea, calling a different 
tool in the `llvm/test/tools/llvm-objcopy/mirror-permissions-*.test` seems like 
violating the layer.



Comment at: llvm/test/tools/llvm-objcopy/ELF/file-permissions.test:1
+# RUN: cp %p/Inputs/dwarf.dwo %t-exe.dwo
+# RUN: chmod a+x %t-exe.dwo

jhenderson wrote:
> llvm-objcopy already has testing for permissions - see 
> mirror-permissions-*.test and respect-umask.test. It seems like a new test 
> file would be a mistake. Furthermore, a casual inspection to me makes it seem 
> like this behaviour is already covered (and working correctly), which makes 
> me think that no new testing is needed for this tool, but please review the 
> existing test and see what you think.
thanks! I didn't notice there is an existing one. Taking a look on these test, 
I think it already covers the cases we aim to test. So no need to add a new one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits