Total response file count limited to 21

Project Infrastructure LLVM Dev List Archives
1

Hi,

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)
return false;

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?

Thanks!

– chris

2

Hi Chris,

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.

Thanks,

Shoaib

3

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.

4

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?

Thanks,
Hans

5

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?

– chris

6

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.

7

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.

– chris

8

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.

9

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.

– chris