[PATCH] D81676: [MSP430] Align the toolchain definition with the TI's msp430-gcc v9.2.0

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko marked 3 inline comments as done.
atrosinenko added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:154
 
+  SmallString<128> MultilibInclude(GCCInstallation.getInstallPath());
+  llvm::sys::path::append(MultilibInclude, "include");
----------------
krisb wrote:
> I'd guard this by `if (GCCInstallation.isValid())` to avoid adding include directories with relative paths if `GCCInstallation.getInstallPath()` is empty.
Fixed.


================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:239
+  Arg *SspFlag = Args.getLastArg(
+      options::OPT_fno_stack_protector, options::OPT_fstack_protector,
+      options::OPT_fstack_protector_all, options::OPT_fstack_protector_strong);
----------------
krisb wrote:
> Is the check for `fno-stack-protector` necessary here? Looks as the checks for 'positive' options should be enough to do the trick.
While the "negative" option is not mentioned at all by the specs:
```
$ msp430-elf-gcc -dumpspecs | grep stack-protector
%{fstack-protector|fstack-protector-all|fstack-protector-strong|fstack-protector-explicit:-lssp_nonshared -lssp}
```
it seems these options are already basically preprocessed by gcc driver:
```
$ msp430-elf-gcc -### empty.o 2>&1 | grep -Eo " -l[a-z_0-9]*" | tr "\n" " "
 -lgcc  -lmul_none  -lc  -lgcc  -lcrt  -lnosys  -lgcc  -lgcc
$ msp430-elf-gcc -### empty.o -fstack-protector 2>&1 | grep -Eo " -l[a-z_0-9]*" | tr "\n" " "
 -lssp_nonshared  -lssp  -lgcc  -lmul_none  -lc  -lgcc  -lcrt  -lnosys  -lgcc  -lgcc
$ msp430-elf-gcc -### empty.o -fstack-protector -fno-stack-protector 2>&1 | grep -Eo " -l[a-z_0-9]*" | tr "\n" " "
 -lgcc  -lmul_none  -lc  -lgcc  -lcrt  -lnosys  -lgcc  -lgcc
```
and they seem to have the usual semantics of "the last one takes effect".


================
Comment at: clang/test/Driver/msp430-toolchain.c:5
+
+// Test for include paths (cannot use -###)
+
----------------
krisb wrote:
> This way looks okay to me, but I believe you should be able to check cc1 command (though -###) for `-internal-isystem`, right?
It works, thank you! Will wait for unit test results on Windows.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81676/new/

https://reviews.llvm.org/D81676