Page MenuHomePhabricator

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

Authored by troyj on Oct 29 2018, 1:56 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

troyj created this revision.Oct 29 2018, 1:56 PM
Herald added subscribers: Restricted Project, llvm-commits. · View Herald TranscriptOct 29 2018, 1:56 PM

+ 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 accepted this revision.Dec 3 2018, 10:55 AM

@troyj, good catch! Thanks, and LGTM.

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

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?

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 22 2019, 12:07 PM
mattd added a comment.EditedMar 22 2019, 8:52 PM

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?

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.

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 added a comment.Apr 1 2019, 11:24 AM

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.

This revision was automatically updated to reflect the committed changes.
mattd added a comment.Apr 1 2019, 8:27 PM

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.