[PATCH] D81676: [MSP430] Align the toolchain definition with the TI's msp430-gcc v8.3.1
Anatoly Trosinenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 22 09:07:38 PDT 2020
atrosinenko marked 3 inline comments as done.
atrosinenko added a comment.
Thank you for the comments. Will send an update soon.
================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:155
SmallString<128> Dir(computeSysRoot());
llvm::sys::path::append(Dir, "include");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
----------------
krisb wrote:
> Seems the driver stops adding `msp430-elf/include` to the default include search paths, instead it adds `$sysroot/include` (or `$gcc-toolchain/include`). As I can see the latter isn't among the default include paths used by TI gcc.
> So, shouldn't we change here to `llvm::sys::path::append(Dir, "msp430-elf", "include");` as well?
Frankly speaking, I was more concerned with linker arguments autodiscovery. Adding `$sysroot/msp430-elf/include` definitely makes sense because that directory contains standard headers.
================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:239
+ ArgStringList &CmdArgs) {
+ if (!Args.hasArg(options::OPT_T)) {
+ if (Args.hasArg(options::OPT_msim)) {
----------------
krisb wrote:
> What about an early exit here?
Agree, this would make this code clearer.
================
Comment at: clang/test/Driver/msp430-toolchain.c:12
// RUN: %clang %s -### -no-canonical-prefixes -target msp430 --sysroot="" 2>&1 \
-// RUN: | FileCheck -check-prefix=CC1 %s
-// CC1: clang{{.*}} "-cc1" "-triple" "msp430"
+// RUN: | FileCheck -check-prefix=DEFAULT-NEG %s
+// DEFAULT-POS: clang{{.*}} "-cc1" "-triple" "msp430"
----------------
krisb wrote:
> How about using a single run line with just `--check-prefixes=DEFAULT-POS,DEFAULT-NEG`?
My goal was to ensure everything marked with `DEFAULT-POS` exists in the input stream. At the same time, none of patterns marked with `DEFAULT-NEG` should exist there.
It would be great to check this in a single run (at least, there would be no chance to run the compiler process twice with slightly different arguments by mistake). From reading the [FileCheck documentation](https://llvm.org/docs/CommandGuide/FileCheck.html) it is not very clear the precise semantics of `--check-prefixes=A,B`. Experimenting with it shows it really behaves **not** in a way I need here.
```name=test.txt
POS: A
POS: B
NEG-NOT: 1
NEG-NOT: 2
```
`FileCheck test.txt --check-prefixes=POS,NEG` does fail for the following input:
```
A
B
1
```
as expected. On the other hand, the following is considered OK:
```
A
1
B
```
As I understand, when an option `--check-prefixes=POS,NEG` is given to `FileCheck`, it just recognizes both prefixes but it uses them in the same pass.
What can really help here is `--implicit-check-not`. According to the same documentation, it does precisely what I need but requires specifying every forbidden pattern in the command line.
Another possibility that I prefer would be to rewrite it like this:
```
// RUN: %clang %s -### -no-canonical-prefixes -target msp430 --sysroot="" > %t 2>&1
// RUN: FileCheck --check-prefix=DEFAULT-POS %s < %t
// RUN: FileCheck --check-prefix=DEFAULT-NEG %s < %t
```
This would still be slightly too verbose but at least the compiler arguments are specified only once and just a single clang process is spawned.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81676/new/
https://reviews.llvm.org/D81676
More information about the cfe-commits
mailing list