[PATCH] Fix clang test failures on MIPS

Hi,

The following clang test cases are failed on MIPS. As far as I can see the same tests are failed on ARM as well (http://lab.llvm.org:8011/builders/clang-native-arm-cortex-a9/builds/1762/steps/test-clang/logs/fail). The attached patch fixes some tests and marks others as “expected fail” on MIPS. Could you please review the patch?

Clang :: Preprocessor/mmx.c
Clang :: Preprocessor/predefined-arch-macros.c

These test check definitions of various i386…x86-64 platforms specific macros. I suggest to add “mips” to the XFAIL list for these tests.

Clang :: Driver/debug-options-as.c

This test depends on integrated assembler. I suggest to turn it on explicitly by the “-integrated-as” option.

Clang :: Tooling/clang-check.cpp
Clang :: Tooling/clang-check-pwd.cpp
Clang :: Tooling/clang-check-args.cpp
Clang :: Tooling/clang-check-builtin-headers.cpp

These tests expect that clang driver runs exactly one compiler job, but integrated assembler is turned off on MIPS by default and the driver runs two jobs. I suggest to add “mips” to the XFAIL list for these tests.

mips-test.patch (3.5 KB)

Hi,

The following clang test cases are failed on MIPS. As far as I can see the
same tests are failed on ARM as well
(http://lab.llvm.org:8011/builders/clang-native-arm-cortex-a9/builds/1762/steps/test-clang/logs/fail).
The attached patch fixes some tests and marks others as "expected fail" on
MIPS. Could you please review the patch?

Clang :: Preprocessor/mmx.c
Clang :: Preprocessor/predefined-arch-macros.c

These test check definitions of various i386..x86-64 platforms specific
macros. I suggest to add "mips" to the XFAIL list for these tests.

For those two tests, could you just force an appropriate triple with
-ccc-host-triple?

Clang :: Driver/debug-options-as.c

This test depends on integrated assembler. I suggest to turn it on
explicitly by the "-integrated-as" option.

Okay.

Clang :: Tooling/clang-check.cpp
Clang :: Tooling/clang-check-pwd.cpp
Clang :: Tooling/clang-check-args.cpp
Clang :: Tooling/clang-check-builtin-headers.cpp

These tests expect that clang driver runs exactly one compiler job, but
integrated assembler is turned off on MIPS by default and the driver runs
two jobs. I suggest to add "mips" to the XFAIL list for these tests.

Okay, but please add a comment noting why you're XFAILing it on MIPS.

-Eli

Hi Simon,

Clang :: Preprocessor/mmx.c
Clang :: Preprocessor/predefined-arch-macros.c

These test check definitions of various i386..x86-64 platforms specific macros.
I suggest to add "mips" to the XFAIL list for these tests.

Clang :: Driver/debug-options-as.c

This test depends on integrated assembler. I suggest to turn it on explicitly
by the "-integrated-as" option.

Clang :: Tooling/clang-check.cpp
Clang :: Tooling/clang-check-pwd.cpp
Clang :: Tooling/clang-check-args.cpp
Clang :: Tooling/clang-check-builtin-headers.cpp

These tests expect that clang driver runs exactly one compiler job, but
integrated assembler is turned off on MIPS by default and the driver runs two
jobs. I suggest to add "mips" to the XFAIL list for these tests.

  I have the same failures on ARM (except clang-check-builtin-headers.cpp since
rc1 doesn't have it) [1].

  Looking for your patch! :wink:

Regards,
chenwj

[1] http://people.cs.nctu.edu.tw/~chenwj/tmp/phase3-regression-test.txt

FWIW, I think this is a real bug that we should fix. Integrated assembler shouldn’t impact these tools, there is probably something we’re failing te set up correctly in the driver and invocation.

I’ve CC-ed Manuel. We should be able to reproduce this by actively using the MIPS triple.

Clang :: Preprocessor/mmx.c
Clang :: Preprocessor/predefined-arch-macros.c

These test check definitions of various i386..x86-64 platforms specific
macros. I suggest to add "mips" to the XFAIL list for these tests.

For those two tests, could you just force an appropriate triple with
-ccc-host-triple?

Yes, it is possible. The appropriate patch "tgt-force.patch" is
attached to this mail. But I'm not absolutely sure that this approach
is correct. If we pass a target triple explicitly, we check predefined
macros on that target only. If we do not pass a target triple, we
check predefined macros on each different target where we run the test
suite.

But if you do not see any problem here and "tgt-force.patch" is okay,
I will commit it.

Clang :: Driver/debug-options-as.c

This test depends on integrated assembler. I suggest to turn it on
explicitly by the "-integrated-as" option.

Okay.

Commited at r156063.

tgt-force.patch (24.3 KB)

Do you suggest to keep these test failed while the bug is not fixed?
It's not a problem - just want to clarify.

By the way, if somebody give me a short intro to the tooling library
and "clang-check" tool, I would be happy to investigate the bug and
probably fix it.

This was my hope, and why I CC’ed Manuel so he could provide some insight into why this might be failing…

My understanding is that the tooling library wishes to only run a single compilation job, and is failing to properly set up the driver/invocation for that, leaving the external assembler job also in the pipeline. I’m not of the exact location of this code, but I think Manuel has seen related problems recently anyways.

Yep, we need to add a hook to adjust the compile command lines for
running a single syntax only job.
My idea was to add a hook that by default does that syntax only
modification and allow users to specify different hooks in case they
know what they're doing ...

Cheers,
/Manuel

Just want to be sure that I understand you properly before start
coding. Does the solution below conform your original idea?

1. Add virtual function processCommandLine(StringRef FilePath,
std::vector<std::string> &CommandLine) to the ClangTool class. This
function adds/removes/replaces command line arguments if necessary.
2. Call this function from ClangTool::run() method before
ToolInvocation instance creation to adjust command line.
3. Create class ClangSyntaxCheckTool. It inherits class ClangTool and
overrides processCommandLine() method. The new method adds
"-fsyntax-only" argument to the command line.
3. Use ClangSyntaxCheckTool instead of ClangTool in the clang-check.

Ping.

I actually thought that preparing the command line for syntax checking
would be the default, as all tools I've written so far need it, and
the only one I've seen using something different was Nick when he was
playing around with mapreducing code generation.

I haven't put too much thought into the interface yet, but I'd imagine
something along the lines of
class ArgumentAdjuster {
public:
  vector<std::string> Adjust(const vector<std::string> &Arguments) = 0;
};

class ClangSyntaxOnlyAdjuster : public ArgumentAdjuster { ... };

Then, for people who have command lines that are for gcc for some reasons:
class GccToClangAdjuster : public ArgumentAdjuster { ... };

Then we can install a ClangSyntaxOnlyAdjuster per default and add a
method to ClangTool:
void setArgumentAdjuster(ArgumentAdjuster *Adjuster);

I think the argument adjusting is orthogonal to the tool, so I'd
rather avoid inheritance on the ClangTool level.

Thoughts?
/Manuel

Now I see your point and like this approach. I will implement and send
a patch for review.

Thx! Looking forward to it :slight_smile:

It always bothered me a bit, so since this is going to be adjusted…

Is there a fundamental reason to reason in terms of string here ?

Or more accurately, I understand that parsing the command line requires strings in input, but why should it produce strings in output, instead of a more canonical form ?

– Matthieu

Do you have a proposal what you mean with a 'more canonical form' here?

Cheers,
/Manuel

Some of the refactoring that James and I have been talking about to the driver would directly enabled this – specifically modeling everything as serialization and deserialization of options using the table-generated parsers.

Given such a model, you could use the driver to load the arguments, manipulate them, and even if you needed to write them back out in textual form, do so.

Any baby steps we can take in that direction seem good.

So we could of course do:
typedef std::vector<std::string> CommandLineArguments;
in order to remember that we want something typed there... but I'm not
sure how much that helps for now.

Cheers,
/Manuel

Hi,

adjusters.patch (7.44 KB)

Some thoughts:
- I'd rather have us always install a ClangSyntaxOnlyAdjuster by
default in ClangTool; I have experience with the variant that requires
tools to add it always, and then you basically have 99% of the tools
adding the syntax only adjuster, which seems like a waste
- I'd also require an ArgumentsAdjuster to be set; not modifying the
arguments is actually an unexpected use case - for running stuff next
to a compile, we should encourage the use of clang-plugins :slight_smile:
- just adding -fsyntax-only is not enough, we'll also need to weed out
options that generate output (I have some stuff ready, I'm happy to
put that in later if you want to add a FIXME)
- there's a lost looking empty FIXME next to the ArgsAdjuster member...

Thanks for working on this!
/Manuel

Thanks for the comments. New patch is attached.

adjusters2.patch (6.67 KB)