Page MenuHomePhabricator

[clang][tooling] Accept Clang invocations with multiple jobs
ClosedPublic

Authored by jansvoboda11 on Jul 9 2021, 5:19 AM.

Details

Summary

When -fno-integrated-as is passed to the Clang driver (or set by default by a specific toolchain), it will construct an assembler job in addition to the cc1 job. Similarly, the -fembed-bitcode driver flag will create additional cc1 job that reads LLVM IR file.

The Clang tooling library only cares about the job that reads a source file. Instead of relying on the fact that the client injected -fsyntax-only to the driver invocation to get a single -cc1 invocation that reads the source file, this patch filters out such jobs from Compilation automatically and ignores the rest.

This fixes a test failure in ClangScanDeps/headerwithname.cpp and ClangScanDeps/headerwithnamefollowedbyinclude.cpp on AIX reported here: https://reviews.llvm.org/D103461#2841918 and clang-scan-deps failures with -fembed-bitcode.

Depends on D106788.

Diff Detail

Event Timeline

dexonsmith added inline comments.Jul 9 2021, 10:48 AM
clang/lib/Tooling/Tooling.cpp
122–125

Seems like this could (unexpectedly?) let through a command with multiple -cc1s (if I'm reading the code right). Consider these two cases:

% clang -x c a.c b.c -c -fno-integrated-as -###
% clang -arch x86_64 -arch arm64 -x c /dev/null -c -fno-integrated-as -###

The first compiles two separate files; the second compiles a file with two separate architectures. But IIUC, the logic below doesn't expect this and will now let them through (ultimately, there's no reason scan-deps couldn't be updated to handle this! but that's not in scope for this patch, and maybe doesn't apply generally to clang-tooling).

Also, I'm not sure it's good that it was rejected, but I think the previous code would reject the following command-line and the new code will accept it:

% clang -x c /dev/null -###

I wonder if it'd be simpler to search for a supported action, and reject if zero or multiple are found. Something like:

const driver::Command *Cmd = nullptr;
for (const Action *A : Actions) {
  A = lookThrough(A);
  if (!shouldIgnore(*A)) // ignore some actions...
    continue;
  if (shouldReject(*A)) // if needed, error on some actions...
    return error(...);

  if (Cmd) // error if we hit a second candidate
    return error(...);
  Cmd = cast<driver::Command>(*A);
}
if (!Cmd) // error if we found no candidate
  return error(...);
jansvoboda11 retitled this revision from [clang][tooling] Accept Clang invocations with "-fno-integrated-as" to [clang][tooling] Accept Clang invocations with multiple cc1 jobs.
jansvoboda11 edited the summary of this revision. (Show Details)

Handle cases with multiple input files and architectures, add tests.

This revision is now accepted and ready to land.Jul 14 2021, 1:41 PM
jansvoboda11 retitled this revision from [clang][tooling] Accept Clang invocations with multiple cc1 jobs to [clang][tooling] Accept Clang invocations with multiple jobs.Jul 15 2021, 1:36 AM

Also ignore cc1 jobs that have inputs that are not source files.

jansvoboda11 requested review of this revision.Jul 26 2021, 5:23 AM

Requesting re-review, since there are two changes:

  • -cc1 commands that don't read a source file are ignored (e.g. jobs generated by -fembed-bitcode),
  • test now have -target arm64-apple-macosx11.0.0 to ensure things work platforms that don't have external assembler.
dexonsmith accepted this revision.Jul 26 2021, 11:23 AM

Seeing the -fembed-bitcode case made me think of -save-temps. I think this will work since -x cpp-output should return false for isSrcFile()... but probably worth adding a test. LGTM assuming the test passes.

This revision is now accepted and ready to land.Jul 26 2021, 11:23 AM

Seeing the -fembed-bitcode case made me think of -save-temps. I think this will work since -x cpp-output should return false for isSrcFile()... but probably worth adding a test. LGTM assuming the test passes.

Thanks for pointing that out. Things work as expected, committing with the added test.

This revision was landed with ongoing or failed builds.Jul 27 2021, 1:48 AM
This revision was automatically updated to reflect the committed changes.