This is an archive of the discontinued LLVM Phabricator instance.

Explicitly detect recursive response files
ClosedPublic

Authored by chrisglover on Jun 2 2019, 7:55 PM.

Details

Summary

Previous detection relied upon an arbitrary hard coded limit of 21 response files, which some code bases were running up against.

The new detection maintains a stack of processing response files and explicitly checks if a newly encountered file is in the current stack. Some bookkeeping data is necessary in order to detect when to pop the stack.

Diff Detail

Event Timeline

chrisglover created this revision.Jun 2 2019, 7:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2019, 7:55 PM
smeenai added inline comments.Jun 3 2019, 10:18 PM
llvm/lib/Support/CommandLine.cpp
1110

Argv.size() can increase, so we could end up potentially popping this off, right? Perhaps something like std::numeric_limits<size_t>::max (or just -1 if you don't feel like being wordy) would be better?

1136

Needs to be clang-formatted. You can use git clang-format to format your entire commit. See https://llvm.org/docs/GettingStarted.html#sending-patches

1157

@rsp file?

I'd say something like "Increase the end of all active records by the number of newly expanded arguments, minus the response file itself"

llvm/unittests/Support/CommandLineTest.cpp
857

Could you expand this test? In particular, we should ensure that we don't have any recursive expansion at all, and we should also test mutual recursion.

chrisglover marked 4 inline comments as done.Jun 4 2019, 6:01 AM
chrisglover added inline comments.
llvm/lib/Support/CommandLine.cpp
1110

If I got this correct, this particular node will never be popped off. There's an assert at the end of the function that checks that the value of this node did indeed expand out to the size of the argument list
assert(Argv.size() == FileStack.back().End);
But, when I think about it, this assert isn't checking any particular post-condition, it's just validating that the algorithm is correct. For this kind of code, which is a bit complex, I feel like this is reasonable choice to defend against future changes, but really it comes down to a philosophical choice that I'm not strongly attached to one way of the other.

1136

Ack.

1157

Fair. I'll use this wording.

llvm/unittests/Support/CommandLineTest.cpp
857

Good point, especially the mutual recursion. I'll add that.

chrisglover updated this revision to Diff 203058.EditedJun 4 2019, 7:29 PM
  • Reworded comment based on review feedback
  • Updated unit test to explicitly ensure we never expand recursive response files
  • Ran git clang-format
  • Added one more test to make sure non-recursive, but repeated response files continue to work.
jyknight accepted this revision.Jun 5 2019, 11:36 AM

I like it. Some minor suggestions for a few extra comments, but other than that LGTM.

llvm/lib/Support/CommandLine.cpp
1106

Could use a comment as to what this represents -- the position of the last argument that came (including recursively) from the named file.

1114

Also can use a comment -- drop any files from the stack if we've finished their arguments.

1166

Worth putting a comment here as to why this is the case.

This revision is now accepted and ready to land.Jun 5 2019, 11:36 AM
smeenai added inline comments.Jun 5 2019, 12:02 PM
llvm/lib/Support/CommandLine.cpp
1110

Yeah, I understand now. I forgot to account for the dummy entry also having its End incremented in the loop below.

1166

Yeah, this would address the confusion I had :)

chrisglover marked 4 inline comments as done.Jun 5 2019, 12:54 PM
chrisglover added inline comments.
llvm/lib/Support/CommandLine.cpp
1106

How about

// To detect recursive response files, we maintain a stack of files and the
// position of the last argument in the file. This position is
// updated dynamically as we recursively expand files.
1114

How about

  
// Passing the end of a file's argument list, so we can remove it from the
// stack.
1166

Does this make sense? Too wordy?

// If successful, the top of the file stack will mark the end of the Argv
// stream. A failure here indicates a bug in the stack popping logic above.
// Note that FileStack may have more than one element at this point because we
// don't have a chance to pop the stack when encountering recursive files at
// the end of the stream, so seeing that doesn't indicate a bug.

Comment updates SGTM.

llvm/lib/Support/CommandLine.cpp
1166

SGTM. Probably worth changing the assert to also check size > 0:
assert(FileStack.size() > 0 && Argv.size() == FileStack.back().End);

  • Added FileStack.size() > 0 check in assert at end of function.

If this looks good, someone else will need to commit as I don't have commit access.

Sorry, I thought James was gonna commit this, but I'll go ahead and do so.

This revision was automatically updated to reflect the committed changes.