I recently hit this on a project using a build system that relies heavily on nested response files. We found we could only have 21 response files total before getting errors related to the unexpanded response files. I tracked it down to this code in llvm/lib/Support/CommandLine.cpp
// If we have too many response files, leave some unexpanded. This avoids
// crashing on self-referential response files.
if (RspFiles++ > 20)
This seems rather arbitrary and in tests I was able to increase it to 200 reliably, which we could do locally for now, but I feel there must be a better way to handle this by tracking processed response files instead of just bailing like this. Or am I missing something?
I fixed this in https://reviews.llvm.org/D60631. If you’re using a released version of clang you’ll have to wait for 9.0 though.
Actually, sorry, my fix was for the case where you had other arguments beginning with @ that weren’t response files, whereas yours has actual response files, so my patch won’t help there. CCing Reid and Hans, who did a bunch of the implementation in this area.
It seems reasonable to have an expansion limit, but maybe 20 is too low.
Chris: You say you're relying heavily on nested response files. Do you
have a feel for what would be a reasonable limit that wouldn't
interfere with your use case?
It’s hypothetically unbounded, of course, but I could see 50 being in range, so 100 would probably be fool proof.
And yes, it seems reasonable to have a limit, but maybe a message when the limit is a hit and a flag to override it would be an acceptable approach?
IMO, a limit of at most 20 nested response files would make a lot more sense than 20 total response files. I don’t think the total should really have a limit at all.
Since we expand the files in place while iterating over the arglist, we’d need to keep a separate array listing the end-offset of each file we’re currently nested within, and update the offsets with every expansion of arguments. That’s a bit more complex than just an integer count of number of files seen so far, but it should be implementable completely within the ExpandResponseFiles function, and I don’t think it’d be that tricky.
Seems reasonable to me. And would absolutely solve the issue here (I think we only nest 2 or 3 levels deep).
If this is an agreed upon solution, I’d be happy to attempt to implement it.
Instead of bumping up the threshold, can we detect a cycle in reference files? We have
llvm::sys::fs::equivalent to determine whether two file descriptors are for the same file or not. It doesn’t seem too hard to detect a cycle using that function.
This was my initial thought. In my opinion, there should be no artificial limit, but I can also see a reasonable argument to have such a limit, so I don’t really mind either way.