Page MenuHomePhabricator

[builtins] Improve compatibility with 16 bit targets
ClosedPublic

Authored by atrosinenko on Jun 8 2020, 9:09 AM.

Details

Summary

Some parts of existing codebase assume the default int type to be (at least) 32 bit wide. On 16 bit targets such as MSP430 this may cause Undefined Behavior or results being defined but incorrect.

Diff Detail

Event Timeline

atrosinenko created this revision.Jun 8 2020, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 9:09 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Jun 8 2020, 9:35 AM
compiler-rt/lib/builtins/fp_lib.h
49

Why is this needed?

atrosinenko added inline comments.Jun 8 2020, 11:11 AM
compiler-rt/lib/builtins/fp_lib.h
49

First of all, I will compile the examples with gcc because right now I most probably don't have a TI sysroot for MSP430 compatible with the mainline clang and testing with my locally patched llvm+clang seems even less correct than with gcc.

The problem is that the definition, according to the gcc documentation is int __builtin_clz (unsigned int x). This builtin is defined with the "default size" int argument, not uint32_t or int32_t. Yes, there are ints in the specifications that are not exactly int but something "expected to be int" actually expressed via machine modes. But here it looks like it is actually an int:

clzsi_test.c
#include <stdio.h>
#include <stdint.h>

typedef uint32_t rep_t;

static int rep_clz(rep_t a) { return __builtin_clz(a); }

int main()
{
  printf("sizeof(int) = %d\n", (int)sizeof(int));
  printf("first:  %d\n", rep_clz(0x1000));
  printf("second: %d\n", rep_clz(0x10000));
  printf("third:  %d\n", rep_clz(0x100000));
}

When compiled and launched on x86_64 Linux host, it prints the expected results:

$ gcc -Wall clzsi_test.c -o clzsi_test && ./clzsi_test 
sizeof(int) = 4
first:  19
second: 15
third:  11

Let's compile and run it with

$ $sysroot/bin/msp430-elf-gcc --version
msp430-elf-gcc (Mitto Systems Limited - msp430-gcc 8.3.1.25) 8.3.1
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Then we get the following (the -msim option is necessary to run the compiled binary with msp430-elf-run simulator):

$ $sysroot/bin/msp430-elf-gcc -msim -Wall clzsi_test.c -o clzsi_test && $sysroot/bin/msp430-elf-run ./clzsi_test 
sizeof(int) = 2
first:  3
second: 16
third:  16

So, these are expected results for clz(i16 arg) (with the third line being an UB at all) but not the results expected for rep_t-sized argument. (When I comment out the third printf, other output that now is expected to be UB-free remains unchanged)

When I change rep_clz to use __builtin_clzl, the output on MSP430 simulator differs from the host one just in the sizeof... line, as expected.

sizeof(int) = 2
first:  19
second: 15
third:  11

Of course, now it will output not the expected values on x86_64, so using clzsi instead of a hardcoded name.

MaskRay added inline comments.Jun 8 2020, 4:05 PM
compiler-rt/lib/builtins/fp_lib.h
49

Where is this clzsi defined? It seems that clzsi is provided by either __builtin_clz or __builtin_clzl, but the desired version is a customized one.

efriedma added inline comments.Jun 8 2020, 4:29 PM
compiler-rt/lib/builtins/fp_lib.h
49

Where is this clzsi defined?

compiler-rt/lib/builtins/int_types.h

atrosinenko added inline comments.Jun 9 2020, 4:35 AM
compiler-rt/lib/builtins/fp_lib.h
49

Just to have these defines here for a reference:

compiler-rt/lib/builtins/int_types.h
typedef int32_t si_int;
typedef uint32_t su_int;
#if UINT_MAX == 0xFFFFFFFF
#define clzsi __builtin_clz
#define ctzsi __builtin_ctz
#elif ULONG_MAX == 0xFFFFFFFF
#define clzsi __builtin_clzl
#define ctzsi __builtin_ctzl
#else
#error could not determine appropriate clzsi macro for this system
#endif
aykevl added a comment.Jun 9 2020, 5:48 AM

I tested this patch against my local testing system to make sure it didn't break anything, and I get the same number of failures (most of which are due to a missing complex.h file, which is unrelated). So it looks fine from my point of view, although I know not enough about this code to LGTM it.
Note that double float (float128) doesn't work in my setup, so I can't test changes related to that.

Our out-of-tree target also have 16 bit ints and we've had to change a whole bunch of builtin_clz to clzsi and builtin_ctz to ctzsi to make things work so this patch is a step in the right direction for us too.

Rebase onto working upstream commit to (hopefully) make tests pass for my patch.

Ping.

Those flaky test failures seems to be due to ld.lld being not built from source as part of testing compiler-rt/-only patches.

Those flaky test failures seems to be due to ld.lld being not built from source as part of testing compiler-rt/-only patches.

That should be something we can fix in the build system. compiler-rt/test/CMakeLists.txt has a list of executables which the tests depend on. If that list isn't complete, it should be updated.

Those flaky test failures seems to be due to ld.lld being not built from source as part of testing compiler-rt/-only patches.

That should be something we can fix in the build system. compiler-rt/test/CMakeLists.txt has a list of executables which the tests depend on. If that list isn't complete, it should be updated.

@efriedma

Agree, but another question: is it acceptable to have them disabled when retesting compiler-rt? So, maybe lld subproject have to be explicitly enabled as well for compiler-rt-only tests...

@goncharov

Meanwhile, looking at the latest Buildkite build for this review, we have:

artifacts/CMakeCache.txt (Linux)
...
LLVM_ENABLE_PROJECTS:STRING=clang;compiler-rt;llvm
...

and

artifacts/CMakeCache.txt (Windows)
...
LLVM_ENABLE_PROJECTS:STRING=clang;llvm
...

So, isn't Windows build here just wasting CPU time?

Another my concern here is that the build plan pre-merge checks (buildkite) is expressed with just a single "Make HTTP request" step with "When Complete: Wait For Message" (unlike the pre-merge checks used for the initial upload). So, this build failed like this (see the message tab):

216557	merge_guards_bot	fail	Consumed	Thu, Jun 18, 16:13
216556	merge_guards_bot	working		Thu, Jun 18, 16:13
216538	merge_guards_bot	working		Thu, Jun 18, 15:51

In D81282: [builtins] Move more float128-related helpers to GENERIC_TF_SOURCES list, on the other hand, there is seemingly passed build. Its messages are

216370	merge_guards_bot	fail		Thu, Jun 18, 13:28
216365	merge_guards_bot	working		Thu, Jun 18, 13:25
216343	merge_guards_bot	pass	Consumed	Thu, Jun 18, 12:53
216342	merge_guards_bot	working		Thu, Jun 18, 12:52

So, it looks like now the compiler-rt-only pre-merge check cannot pass at all, but can be marked Passed on re-upload only based on what builder (Linux or Windows) will send its reply first.

efriedma accepted this revision.Jun 22 2020, 12:11 PM

LGTM

We don't need to block this on the pre-merge checks.

This revision is now accepted and ready to land.Jun 22 2020, 12:11 PM

@goncharov

Meanwhile, looking at the latest Buildkite build for this review, we have:

artifacts/CMakeCache.txt (Linux)
...
LLVM_ENABLE_PROJECTS:STRING=clang;compiler-rt;llvm
...

and

artifacts/CMakeCache.txt (Windows)
...
LLVM_ENABLE_PROJECTS:STRING=clang;llvm
...

So, isn't Windows build here just wasting CPU time?

@atrosinenko: could you please clarify what exactly is wrong? Windows not building 'compiler-rt'?

Another my concern here is that the build plan pre-merge checks (buildkite) is expressed with just a single "Make HTTP request" step with "When Complete: Wait For Message" (unlike the pre-merge checks used for the initial upload). So, this build failed like this (see the message tab):

216557	merge_guards_bot	fail	Consumed	Thu, Jun 18, 16:13
216556	merge_guards_bot	working		Thu, Jun 18, 16:13
216538	merge_guards_bot	working		Thu, Jun 18, 15:51

In D81282: [builtins] Move more float128-related helpers to GENERIC_TF_SOURCES list, on the other hand, there is seemingly passed build. Its messages are

216370	merge_guards_bot	fail		Thu, Jun 18, 13:28
216365	merge_guards_bot	working		Thu, Jun 18, 13:25
216343	merge_guards_bot	pass	Consumed	Thu, Jun 18, 12:53
216342	merge_guards_bot	working		Thu, Jun 18, 12:52

So, it looks like now the compiler-rt-only pre-merge check cannot pass at all, but can be marked Passed on re-upload only based on what builder (Linux or Windows) will send its reply first.

Yes, there is a single request to buildkite and a single response that happens at "report" step after both windows and linux build are done, e.g. https://buildkite.com/llvm-project/premerge-checks/builds/898#7efe45eb-ac96-40aa-ba91-494a5812ec88

The latter "successfull" build was a temporary misconfiguration of the build scripts. 1. I believe I have restarted all affected builds but clearly missed some. 2. I have created https://github.com/google/llvm-premerge-checks/issues/206 to prevent this from happening,

could you please clarify what exactly is wrong? Windows not building 'compiler-rt'?

Yes. Maybe I misunderstood the intention. It just looked quite strange to me that when only the compiler-rt/ subtree of the repository was changed, pre-merge check was optimizing build by dropping all unrelated subprojects - and compiler-rt was dropped as well.

This revision was automatically updated to reflect the committed changes.