Page MenuHomePhabricator

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

Authored by atrosinenko on Jun 11 2020, 11:15 AM.

Details

Summary

This patch updates the toolchain description for MSP430 target, aligning it with the TI-provided sysroot based on msp430-gcc v9.2.0.

It leaves some features (such as sanitizer runtimes, LTO, etc.) unsupported, trying to translate the remaining parts of the link_command spec description from current GCC version as closely as possible.

It introduces support for GCC -msim option to Clang that simplifies building msp430 binaries to be run on a simulator (such as for unit testing purposes).

This patch contains updated unit tests to prevent silent changing of the behavior. Its current behavior can be manually tested as follows:

  • Compile and run on the simulator: compiles successfully, runs as expected, terminates cleanly
$ /path/to/bin/clang -target msp430 --sysroot=$sysroot test.c -o test -msim
$ $sysroot/bin/msp430-elf-run ./test
N = 1
  • Compile for a real MCU: links successfully, can be run with MSP430 Emulator after manually setting pc to address written at the reset vector (0xFFFE)
$ /path/to/bin/clang -target msp430 --sysroot=$sysroot -mmcu=msp430g2553 blink.c -o blink -I $sysroot/include

Please note that in the latter case msp430-elf-gcc requires -I... option as well.

Current state:

  • can run simple programs on a simulator built into msp430-elf-gdb
  • can link a program by passing just a --sysroot=/path/to/msp430-gcc/binary/distrib
  • tested on third-party MSP430 Emulator, see above
  • not tested on a real hardware
  • may require further adjustment of --gcc-toolchain option handling
test.c
#include <stdio.h>

int main()
{
  printf("N = %d\n", 1);
  return 0;
}

References:

Diff Detail

Unit TestsFailed

TimeTest
7,600 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && env KMP_DISP_NUM_BUFFERS=0 /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp
1,220 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

atrosinenko created this revision.Jun 11 2020, 11:15 AM
atrosinenko edited the summary of this revision. (Show Details)Jun 11 2020, 11:18 AM

fix unit test: add another "-lgcc" and (hopefully) fix path separators on Windows

Fix test once again (replace more literal slashes with regex).

krisb added inline comments.Jun 22 2020, 3:51 AM
clang/lib/Driver/ToolChains/MSP430.cpp
163

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?

247

What about an early exit here?

clang/test/Driver/msp430-toolchain.c
75–115

How about using a single run line with just --check-prefixes=DEFAULT-POS,DEFAULT-NEG?

100

Same here about --check-prefixes

atrosinenko marked 3 inline comments as done.Jun 22 2020, 8:39 AM

Thank you for the comments. Will send an update soon.

clang/lib/Driver/ToolChains/MSP430.cpp
163

Frankly speaking, I was more concerned with linker arguments autodiscovery. Adding $sysroot/msp430-elf/include definitely makes sense because that directory contains standard headers.

247

Agree, this would make this code clearer.

clang/test/Driver/msp430-toolchain.c
75–115

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 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.

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.

Address the review comments.

Now include directories should be set in a much more similar way to msp430-gcc v8.3.1. The test was added as well. The most significant difference is that Clang adds one extra include directory: /path/to/build/dir/lib/clang/11.0.0/include. Hope this would not mess things up too much.

After adjusting the standard include directories, test programs for the simulator can be built with something like this

/path/to/clang -target msp430 --sysroot=$sysroot test.c -o test -msim

without specifying -I flag (just with msp430-elf-gcc).

Still, I am not sure this handles --sysroot and --gcc-toolchain exactly in the expected way. But at least it handles --sysroot=/path/to/distribution/from/TI as suggested in https://clang.llvm.org/docs/CrossCompilation.html

Meanwhile, while rewriting the msp430-toolchain.c to get rid of pairs of identical compiler invocations, I have found a forgotten ...-NEG case. :)

atrosinenko edited the summary of this revision. (Show Details)Jun 23 2020, 1:37 AM

Unit test: fix for path separators on Windows

Meanwhile, msp430-gcc v9.2.0 was released on Jun 12. Will recheck against the new version slightly later.

  • Rebase onto an up-to-date upstream commit
  • Recheck against v9.2.0
  • Misc fixes and improvements
atrosinenko retitled this revision from [MSP430] Align the toolchain definition with the TI's msp430-gcc v8.3.1 to [MSP430] Align the toolchain definition with the TI's msp430-gcc v9.2.0.Jun 30 2020, 8:50 AM
atrosinenko edited the summary of this revision. (Show Details)
krisb added a comment.Jul 6 2020, 2:46 AM

Thank you!
LGTM, except some minor nits below.

clang/lib/Driver/ToolChains/MSP430.cpp
154

I'd guard this by if (GCCInstallation.isValid()) to avoid adding include directories with relative paths if GCCInstallation.getInstallPath() is empty.

239

Is the check for fno-stack-protector necessary here? Looks as the checks for 'positive' options should be enough to do the trick.

clang/test/Driver/msp430-toolchain.c
5

This way looks okay to me, but I believe you should be able to check cc1 command (though -###) for -internal-isystem, right?

atrosinenko marked 3 inline comments as done.Jul 7 2020, 2:24 AM
atrosinenko added inline comments.
clang/lib/Driver/ToolChains/MSP430.cpp
154

Fixed.

239

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".

clang/test/Driver/msp430-toolchain.c
5

It works, thank you! Will wait for unit test results on Windows.

Address the review comments.

Fix path separators in unit test on Windows

Just add the test on -rtlib=compiler-rt handling as everything other is ready for basic clang_rt.builtins support.

Just an automatic rebase to (possibly) fix failing "Apply patch" step.

krisb accepted this revision.Tue, Jul 14, 7:10 AM

Sorry for the delays in response, busy days.
LGTM, thanks!

This revision is now accepted and ready to land.Tue, Jul 14, 7:10 AM

Thank you! Will probably land it tomorrow.

This revision was automatically updated to reflect the committed changes.

After a final retesting, this patch was updated a bit.

The msp430-toolchain.c test was failing in our setup, and I've put together a fix in D84058.