Make instrprof-set-dir-mode test tolerant of group ID

[troyj]

Summary

Hi, I ran into a problem with this test when the source was located in certain directories. The mkdir(2) man page states that the set-group-ID bit is inherited from the parent directory, but this test was written in such a way that it assumes the bit is unset. Whether that assumption is true depends on where the checkout lives, which leads to some people being able to reproduce the problem whereas others cannot. I think the correct fix is to exclude the bit from the check.

Making probinson a reviewer since they reviewed the original test.

[probinson]

+ Matt Davis who wrote the test
Sorry for the delay; feel free to add a 'ping' reply every week or so when you get no action.
Seems right to me but I think Matt would be a better choice to review.

[mattd]

@troyj, good catch! Thanks, and LGTM.

[troyj]

This revision is now accepted and ready to land.Dec 3 2018, 12:55 PM

Been about 3.5 months since accepted. Can this be committed?

[mattd]

In D53832#1439874, @troyj wrote:

This revision is now accepted and ready to land.Dec 3 2018, 12:55 PM

Been about 3.5 months since accepted. Can this be committed?

I'm cool with you landing the patch, as long as a rebase of this is free of any non-trivial merge conflicts. The file this patch updates hasn't been modified since July. So you should be in the clear to just land this :) Thanks!

Edit: Do you have commit access, or do you need someone else to land this for you?

[troyj]

I do not have commit access. I probably should request it at some point, but I'm fine with someone else merging this for me.

[mattd]

In D53832#1443179, @troyj wrote:

I do not have commit access. I probably should request it at some point, but I'm fine with someone else merging this for me.

Ah, gotcha. Could you simply rebase the patch, or acknowledge the new LLVM license, as your patch was written prior to the license change.
https://lists.llvm.org/pipermail/llvm-dev/2019-January/129069.html

Otherwise, if you are committing a patch for someone else, please ask them
to explicitly acknowledge that it is under the new license. An easy way to
do this is by just asking them to rebase the patch.

I can try to land this shortly (I just pulled it down and tested it).

[troyj]

Could you simply rebase the patch, or acknowledge the new LLVM license, as your patch was written prior to the license change.

My organization (Cray Inc) already signed the new license agreement, so I'm not sure what you need here. I can write that I acknowledge the new LLVM license, if that helps.

[mattd]

In D53832#1450240, @troyj wrote:

Could you simply rebase the patch, or acknowledge the new LLVM license, as your patch was written prior to the license change.

My organization (Cray Inc) already signed the new license agreement, so I'm not sure what you need here. I can write that I acknowledge the new LLVM license, if that helps.

Thanks! That's fine. I just happened to read the updated license details about committing other's code, and figured I'd play it safe by asking you.

compiler-rt/trunk/test/profile/instrprof-set-dir-mode.c

static int test(unsigned Mode, const char *TestDir) {
   if (!Dir)
     return -1;
   __llvm_profile_set_dir_mode(Mode);
   __llvm_profile_recursive_mkdir(Dir);
 
   if (Mode != __llvm_profile_get_dir_mode())
     Ret = -1;
   else {
+    // From 'man mkdir':
+    // "If the parent directory has the set-group-ID bit set, then so will the
+    // newly created directory."  So we mask off S_ISGID below; this test cannot
+    // control its parent directory.
     const unsigned Expected = ~umask(0) & Mode;
     struct stat DirSt;
     if (stat(Dir, &DirSt) == -1)
       Ret = -1;
-    else if (DirSt.st_mode != Expected) {
+    else if ((DirSt.st_mode & ~S_ISGID) != Expected) {
       printf("Modes do not match: Expected %o but found %o (%s)\n", Expected,
              DirSt.st_mode, Dir);
       Ret = -1;
     }
   }
 
   free(Dir);
   return Ret;
 }
 
 int main(void) {
   if (test(S_IFDIR | 0777, TESTPATH "/foo/bar/baz/") ||
       test(S_IFDIR | 0666, TESTPATH "/foo/bar/qux/"))
     return -1;
   return 0;
 }