Page MenuHomePhabricator

Driver must return non-zero code on errors in command line
ClosedPublic

Authored by sepavloff on May 9 2017, 11:38 AM.

Details

Summary

Now if clang driver is given wrong arguments, in some cases it
continues execution and returns zero code. This change fixes this
behavior.

The fix revealed some errors in clang test set.

File test/Driver/gfortran.f90 added in r118203 checks forwarding
gfortran flags to GCC. Now driver reports error on this file, because
the option -working-directory implemented in clang differs from the
option with the same name implemented in gfortran, in clang the option
requires argument, in gfortran does not.

In the file test/Driver/arm-darwin-builtin.c clang is called with
options -fbuiltin-strcat and -fbuiltin-strcpy. These option were removed
in r191435 and now clang reports error on this test.

File arm-default-build-attributes.s uses option -verify, which is not
supported by driver, it is cc1 option.

Similarly, the file split-debug.h uses options -fmodules-embed-all-files
and -fmodule-format=obj, which are not supported by driver.

Other revealed errors are mainly mistypes.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff created this revision.May 9 2017, 11:38 AM
rsmith added a subscriber: rsmith.May 9 2017, 2:44 PM

Thank you, some of these test typos are ... alarming. =)

A couple of the test updates don't look quite right, but this mostly looks great.

test/Driver/amdgpu-features.c
1 ↗(On Diff #98328)

This should say -o -

5 ↗(On Diff #98328)

Likewise.

test/Driver/split-debug.h
6–13 ↗(On Diff #98328)

These parts of the test don't make sense: the -fmodule-format=obj and -emit-module are -cc1 options, so testing how the driver deals with them doesn't really make a lot of sense. I would suggest deleting the highlighted region of this test rather than making it test the same thing three times.

sepavloff updated this revision to Diff 98395.May 9 2017, 9:20 PM

Addressed reviewer's notes.

sepavloff marked 3 inline comments as done.May 9 2017, 9:20 PM
rsmith accepted this revision.May 10 2017, 2:29 PM

LGTM

This revision is now accepted and ready to land.May 10 2017, 2:29 PM
This revision was automatically updated to reflect the committed changes.
sepavloff reopened this revision.May 14 2017, 11:06 AM
This revision is now accepted and ready to land.May 14 2017, 11:06 AM
sepavloff updated this revision to Diff 98930.May 14 2017, 11:08 AM

Added missed case

The patch missed a case when Compilation object is created during work of
clang based tool, it resulted in fail of the test 'clang-tidy/diagnostic.cpp'.
This addition to the patch add proper checks, hopefully, to all invocations of
'Driver::BuildCompilation'. Small changes are required in clang-tools-extra,
they are tracked by D33173.

alexfh added a subscriber: alexfh.May 15 2017, 4:54 AM
alexfh added inline comments.
lib/Tooling/CompilationDatabase.cpp
208 ↗(On Diff #98930)

This code is used as a library not only for command-line tools. Directly using stderr is wrong in many use cases of the Tooling library. It should instead somehow let the user of the library get these errors via a provided DiagnosticConsumer. Not sure how to do this here without a more careful reading of the code, but wanted to let you know that this change causes a regression at least for clang-tidy (and likely for many other Clang tools).

sepavloff added inline comments.May 17 2017, 5:16 AM
lib/Tooling/CompilationDatabase.cpp
208 ↗(On Diff #98930)

This function does not have a way to report error, so its interface needs to be changed first. The change D33272 implements such modification.

sepavloff updated this revision to Diff 99273.May 17 2017, 5:21 AM

Moved tooling related code into separate change D33272

This revision was automatically updated to reflect the committed changes.