[PATCH] D62798: Explicitly detect recursive response files

[Chris Glover via Phabricator via llvm-commits]

chrisglover marked 4 inline comments as done.
chrisglover added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1110
+  // the need to check for an empty list.
+  FileStack.push_back({"", Argv.size()});
 
----------------
smeenai wrote:
> `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?
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. 


================
Comment at: llvm/lib/Support/CommandLine.cpp:1136
+    // Check for recursize response files.
+    if (std::any_of(FileStack.begin()+1, FileStack.end(), IsEquivalent)) {
+      // This file is recursive, so we leave it in the argument stream and
----------------
smeenai wrote:
> Needs to be `clang-format`ted. You can use `git clang-format` to format your entire commit. See https://llvm.org/docs/GettingStarted.html#sending-patches
Ack.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1157
+    for (ResponseFileRecord &Record : FileStack) {
+      // All records are now bigger. Don't include the @resp file itself, so -1
+      Record.End += ExpandedArgv.size() - 1;
----------------
smeenai wrote:
> @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"
Fair. I'll use this wording.


================
Comment at: llvm/unittests/Support/CommandLineTest.cpp:857
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
   SmallString<128> TestDir;
----------------
smeenai wrote:
> 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.
Good point, especially the mutual recursion. I'll add that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62798