Page MenuHomePhabricator

getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver
ClosedPublic

Authored by shivanshu3 on Sep 1 2020, 8:32 PM.

Details

Summary

MSVC's cl.exe has a few command line arguments which start with -M such as "-MD", "-MDd", "-MT", "-MTd", "-MP".
These arguments are not dependency file generation related, and these arguments were being removed by getClangStripDependencyFileAdjuster() which was wrong.

Diff Detail

Event Timeline

shivanshu3 created this revision.Sep 1 2020, 8:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 8:32 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
shivanshu3 requested review of this revision.Sep 1 2020, 8:32 PM
zahen added a reviewer: rnk.Sep 2 2020, 7:07 AM
hans added a comment.Sep 2 2020, 7:38 AM

Seems reasonable to me. Just some nits.

clang/lib/Tooling/ArgumentsAdjusters.cpp
29

Instead of manually indexing into the StringRef, the I think the if-statement could be replaced by

if (ArgRef.consume_front("--driver-mode="))
  return ArgRef;
32

I'd drop the Optional and just let the empty StringRef signal not finding the driver mode.

107

If getDriverMode() didn't return Optional, this could be simplified to

bool UsingClDriver = (getDriverMode() == "cl");
110–111

This line and the "All dependency-file options begin with -M" line above seem to contradict each other. Perhaps the could be united somehow.

112

Instead of a bool and if below, I'd suggest if-statements and early continues instead. Breaking up the old if-statement is nice though, and maybe the comment from above about what flags to ignore could be moved down here to those if statements. For example:

// -M flags blah blah
if (Arg.startswith("-M") && !UsingClDriver)
  continue;
// MSVC flags blah blah
if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
  continue;
AdjustedArgs.push_back(Args[i]);
126–127

Need to handle -MT too I suppose

shivanshu3 updated this revision to Diff 289606.Sep 2 2020, 5:40 PM
  • Simplified the implementation of getDriverMode and got rid of the Optional return type.
  • When using the cl driver mode, we do not want to skip the next argument for -MF, -MT, -MQ.
shivanshu3 marked 5 inline comments as done.Sep 2 2020, 5:44 PM
shivanshu3 added inline comments.
clang/lib/Tooling/ArgumentsAdjusters.cpp
112

I realized that with my original change, we would skip the next argument under cl driver mode when -MT was used, which would be wrong. The next argument should only be skipped when IsDependencyFileArg is true. So I think it might be cleaner to keep that extra boolean so the code is easy to read and understand. What do you think?

hans added inline comments.Sep 3 2020, 5:48 AM
clang/lib/Tooling/ArgumentsAdjusters.cpp
112

I think having the boolean variable is still more confusing (it adds more state to keep track of) than just handling the cases with if-statements and early continue.

How about:

// -M flags that take an arg..
if (!UsingClDriver && (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")) {
  ++i;
  continue;
}
// -M flags blah blah
if (!UsingClDriver && Arg.startswith("-M"))
  continue;
// MSVC flags blah blah
if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
  continue;

AdjustedArgs.push_back(Args[i]);

?

shivanshu3 updated this revision to Diff 289793.Sep 3 2020, 1:11 PM
  • Remove the bool IsDependencyFileArg in the implementation of getClangStripDependencyFileAdjuster() to make it simpler.
  • Add an extra argument after -MT in the test case to ensure we do not strip arguments after -MT when using the cl driver mode.
shivanshu3 marked 2 inline comments as done.Sep 3 2020, 1:12 PM
shivanshu3 added inline comments.
clang/lib/Tooling/ArgumentsAdjusters.cpp
112

Yup I agree that looks better :)

shivanshu3 marked an inline comment as done.Sep 3 2020, 3:41 PM

Note that I do not have commit access and this change will have to be committed by someone else on my behalf. Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Sep 8 2020, 1:21 AM
This revision was automatically updated to reflect the committed changes.