[PATCH] Teach Clang how to use response files when calling other tools

[Rafael Auler]

Hi Sean,

I agree that having a flag that triggers action in a far away code is bad, and, as such, I was studying the code to see if I could do a larger refactoring to put all response-file-related stuff in a single place, freeing us from any far away code that is important to response files. However, this turned out to be tougher than I think. Because we need both information from Tool objects and from the Driver (to determine whether the command will actually run, in which case we build a resopnse file), I couldn't centralize everything, but I did centralize the small logic that you requested. Unfortunately, we still have some far away code that is important (in Tools.cpp when we call setInputFiles(), crucial to enable support for file lists in ld64), but I believe that this is the best we can do. Therefore, I ended up with only minor modifications regarding the last patch, although I did address all your concerns.

After some assessment and because the Driver is the top level class that calls the other objects (Compilation, Job etc) to run a command, I put the small logic that determines whether to use response files in a Driver private member function. The reason for this is because we don't need to allocate a response file if we are not running the commands (and there are some code paths that instantiate Driver, Compilation and Job objects just to fetch the arguments, but will never run the commands). Therefore, in the Driver, we are able to accurately determine when the command will actually run, triggering the code that determines where response files are needed just before running the command. Also, the Driver seemed a natural place to put such control logic. On the other hand, I had to remove the constness of the Driver::ExecuteJobs -- notice that this function will now provision resources for response files (their name) and, therefore, can't be const. This is a small change that does not impact any code in the tree -- in fact, no user expected ExecuteJobs to be const.

I also addressed your other (minor comments). Yet, I took a look at the loop you mentioned that it would be important to be cleaned up (in Command::Print) and couldn't determine a good way to refactor this. I saw the logic there, and it seems that it is very particular of what we want when receiving crash reports (skip some arguments and quote others). When you mentioned that this needs to be cleaned up, did you mean to remove such idiosyncrasies and just send all arguments to the crash report, or just a smaller refactoring?

Thanks for putting in time to see this,
Rafael Auler

http://reviews.llvm.org/D4897

Files:
  include/clang/Driver/Driver.h
  include/clang/Driver/Job.h
  include/clang/Driver/Tool.h
  lib/Driver/Compilation.cpp
  lib/Driver/Driver.cpp
  lib/Driver/Job.cpp
  lib/Driver/Tool.cpp
  lib/Driver/Tools.cpp
  lib/Driver/Tools.h
  test/Driver/Inputs/gen-response.c
  test/Driver/response-file.c
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D4897.13422.patch
Type: text/x-patch
Size: 32351 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140908/d9b89185/attachment.bin>