Fix LLVM/clang conversion errors #2473

miri64 · 2018-08-15T11:18:44Z

This fixes some conversion errors one gets when compiling with LLVM/clang. Most of them are caused when a bit-specific type (i.e. uint16_t) is implicitly cast to an enum of smaller value range.

The fix is just an explicit casting of those types to the desired target type.

akosthekiss · 2018-08-15T11:31:15Z

@miri64 Thanks for the PR. Some comments and questions:

Which clang did you use, on what platform, and with which build settings ? (Some similar casting issues were patched by Add explicit casts to enum-to-integer conversions #2467 because of clang 6 strictness, but the here reported casts caused no issues.)
Please, see the Coding guidelines. There are some spacing and line length issues Travis CI will soon warn about. You can also check them locally by running tools/check-vera.sh before submitting.
The signoff is almost OK but you need to drop the angle brackets. See the git log for previous commits. The checker is quite strict on the format. (Check locally with tools/check-signed-off.sh.)

miri64 · 2018-08-15T11:36:14Z

Hi, I will address your comments. Is it okay, if I amend them to the existing commit?

akosthekiss · 2018-08-15T11:40:30Z

@miri64 As for the compiler version / platform / build settings question, just add here a comment. There's no need for putting that in a commit message.

As for the style and signoff issues, the PR will need to have a single correct commit at the end, so it might be easier to simply amend the current one and force push it.

miri64 · 2018-08-15T11:48:47Z

@miri64 As for the compiler version / platform / build settings question, just add here a comment. There's no need for putting that in a commit message.

Yes I was planning to do so, sorry to not having been clear on that. I just need to recheck that before I come back on that ;-).

miri64 · 2018-08-15T11:53:09Z

The clang version I used was 3.8.0 (the one provided with a current Ubuntu 16.04) and I tried it for several Cortex-M platforms. I did not however check if the PR you referred to is fixing the issue, as I was building for the JerryScript version that is fixed to the RIOT pkg. Let me re-check if it is solved in the current jerryscript master.

miri64 · 2018-08-15T12:02:46Z

I amended the changes you asked for.

As for the my previous comment: I rechecked now with a3112ab, but the compile issue with clang 3.8.0 still persists.

akosthekiss · 2018-08-15T12:23:00Z

Interesting, my build of latest master with clang 3.8.0 on an Ubuntu 14.04.5 LTS (x86_64) runs without any diagnostics. Is your build setup considerably different from mine?

CC=clang-3.8 tools/build.py --clean --lto=off --error-messages=on --external-context=on \
  --jerry-debugger=on --line-info=on --logging=on --mem-stats=on --profile=es2015-subset \
  --show-opcodes=on --show-regexp-opcodes=on --snapshot-exec=on --snapshot-save=on \
  --valgrind=on --vm-exec-stop=on --jerry-libc=off --debug

miri64 · 2018-08-15T12:40:58Z

Mh, I can't tell exactly, but here is what I've got as cmake command, when building the examples/javascript application in RIOT with TOOLCHAIN=llvm for BOARD=stm32f4discovery:

cmake -B/home/mlenders/Repositories/RIOT-OS/RIOT/examples/javascript/bin/pkg/stm32f4discovery/jerryscript/riot -H./ \
 -DCMAKE_SYSTEM_NAME=RIOT \
 -DCMAKE_SYSTEM_PROCESSOR="cortex-m4" \
 -DCMAKE_C_COMPILER=clang \
 -DCMAKE_C_COMPILER_WORKS=TRUE \
 -DENABLE_LTO=OFF \
 -DFEATURE_VALGRIND=OFF \
 -DENABLE_ALL_IN_ONE=OFF \
 -DJERRY_LIBC=OFF \
 -DJERRY_LIBM=OFF \
 -DJERRY_CMDLINE=OFF \
 -DEXTERNAL_COMPILE_FLAGS="-D__TARGET_RIOT" \
 -DMEM_HEAP_SIZE_KB=16

miri64 · 2018-08-15T12:45:08Z

Here's more on my versions:

$ clang --version
clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.5 LTS
Release:	16.04
Codename:	xenial

Here's some example output from our CI server

miri64 · 2018-08-15T13:52:37Z

(with the command you provided I can't reproduce either by the way)

akosthekiss · 2018-08-15T13:55:04Z

Let's dig further and deeper if you don't mind. (It's itching that I don't understand what's happening.) Here is what I get when I run the cmake && make command copied from the RIOT CI logs (build directory adapted for local build and some progress messages deleted for the sake of brevity):

$ clang-3.8 --version
clang version 3.8.0-2ubuntu3~trusty5 (tags/RELEASE_380/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 14.04.5 LTS
Release:	14.04
Codename:	trusty

$ cmake -Bbuild -H.  -DCMAKE_SYSTEM_NAME=RIOT  -DCMAKE_SYSTEM_PROCESSOR="cortex-m4"  -DCMAKE_C_COMPILER=clang-3.8  -DCMAKE_C_COMPILER_WORKS=TRUE  -DENABLE_LTO=OFF  -DFEATURE_VALGRIND=OFF  -DENABLE_ALL_IN_ONE=OFF  -DJERRY_LIBC=OFF  -DJERRY_LIBM=OFF  -DJERRY_CMDLINE=OFF  -DEXTERNAL_COMPILE_FLAGS="-D__TARGET_RIOT"  -DMEM_HEAP_SIZE_KB=16
-- The C compiler identification is Clang 3.8.0
-- The ASM compiler identification is Clang
-- Found assembler: /usr/bin/clang-3.8
System is unknown to cmake, create:
Platform/RIOT to use this system, please send your config file to [email protected] so it can be added to cmake
-- Detecting C compiler ABI info
System is unknown to cmake, create:
Platform/RIOT to use this system, please send your config file to [email protected] so it can be added to cmake
-- Detecting C compiler ABI info - done
-- CMAKE_BUILD_TYPE            MinSizeRel
-- CMAKE_C_COMPILER_ID         Clang
-- CMAKE_SYSTEM_NAME           RIOT
-- CMAKE_SYSTEM_PROCESSOR      cortex-m4
[[[snip]]]
-- Configuring done
-- Generating done
-- Build files have been written to: /home/akiss/devel/jerry/jerryscript/build

$ make -Cbuild jerry-core jerry-ext jerry-port-default-minimal
make: Entering directory `/home/akiss/devel/jerry/jerryscript/build'
[[[snip]]]
Linking C static library ../lib/libjerry-core.a
[[[snip]]]
Linking C static library ../lib/libjerry-ext.a
[[[snip]]]
Linking C static library ../../lib/libjerry-port-default-minimal.a
[[[snip]]]
make: Leaving directory `/home/akiss/devel/jerry/jerryscript/build'

The Ubuntu version difference (16.04 vs 14.04) simply shouldn't be the cause of difference as clang is of the same version.

However, I've spotted that the CI logs contain the following line: Building application "riot_javascript" for "nucleo-f401re" with MCU "stm32f4". Are you using a cross clang on the CI? Your local clang 3.8 identifies itself as a native compiler, just like mine: Target: x86_64-pc-linux-gnu. How did you force the stack Ubuntu clang to compile for Cortex-M platforms?

May I ask you to run the cmake and make commands from above on your local machine? Meanwhile, I'll set up an Ubuntu 16.04 VM and see what result it gives for me.

miri64 · 2018-08-15T14:11:10Z

However, I've spotted that the CI logs contain the following line: Building application "riot_javascript" for "nucleo-f401re" with MCU "stm32f4". Are you using a cross clang on the CI? Your local clang 3.8 identifies itself as a native compiler, just like mine: Target: x86_64-pc-linux-gnu. How did you force the stack Ubuntu clang to compile for Cortex-M platforms?

AFAIK we are using the stock Ubuntu clang. I did not install a cross-compile version on my local machine and I am still able to flash and run clang built binaries on actual hardware. Our clang configuration you can find in this file, however I myself aren't that deep into our build environment, I'm just the one who stumbled over this issue.

May I ask you to run the cmake and make commands from above on your local machine?

In that case I get the same output as you 🤔

akosthekiss · 2018-08-15T15:11:21Z

Okay, I've tracked down the issue. The warnings are caused by the -fshort-enums compiler option, set in CFLAGS here. For future reference, my reproduction case is:

CC=clang-3.8 CFLAGS="-fshort-enums" \
tools/build.py --clean --lto=off --error-messages=on --external-context=on \
  --jerry-debugger=on --line-info=on --logging=on --mem-stats=on --profile=es2015-subset \
  --show-opcodes=on --show-regexp-opcodes=on --snapshot-exec=on --snapshot-save=on \
  --valgrind=on --vm-exec-stop=on --jerry-libc=off --debug

Notes:

The issue can also be triggered if cmake and make are called directly.
The issue is neither Cortex-M/architecture specific nor OS-specific. The precision loss warnings/errors are hit on x86-64/Linux, x86-64/macOS (no need to set CC), and arm/baremetal (CFLAGS="-target arm-none-eabi -fshort-enums" and --cmake-param="-DCMAKE_C_COMPILER_WORKS=TRUE"), too.

Note to self: stock clang can cross compile to arm just fine with -target.

miri64 · 2018-08-15T15:21:25Z

So how do we proceed?

akosthekiss · 2018-08-15T15:39:41Z

Now, I'll fetch your commit and try it out locally to see if everything is fine, as we don't have a CI for this build setting. (Most probably we wont have that in the future either as -fshort-enums breaks binary compatibility. This also means that developers wont have easy ways to detect the need for such casts so they may/will break future RIOT jerryscript builds. Looking forward to more PRs in that case.)

akosthekiss · 2018-08-15T15:48:33Z

I still get precision loss errors:

jerryscript/jerry-core/ecma/operations/ecma-objects.c:1790:47: error: implicit conversion loses integer precision: 'uint16_t' (aka 'unsigned short')
      to 'lit_magic_string_id_t' [-Werror,-Wconversion]
          return ext_obj_p->u.pseudo_array.u1.class_id;
          ~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
jerryscript/jerry-core/ecma/operations/ecma-typedarray-object.c:919:68: error: implicit conversion loses integer precision: 'uint16_t'
      (aka 'unsigned short') to 'lit_magic_string_id_t' [-Werror,-Wconversion]
  lit_magic_string_id_t class_id = ext_object_p->u.pseudo_array.u1.class_id;
                        ~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
1 error generated.

Please, run the command in my above comment, it enables (close to) all macro-guarded code paths and fix the revealed issues (if there are any on top of these two).

miri64 · 2018-08-15T15:50:36Z

This also means that developers wont have easy ways to detect the need for such casts so they may/will break future RIOT jerryscript builds.

Since RIOT fixes jerryscript to one version, that shouldn't be so critical. Only if we update jerryscript, the maintainer might need to update upstream.

miri64 · 2018-08-15T15:53:05Z

Should be fixed now.

akosthekiss · 2018-08-15T16:20:37Z

jerry-core/ecma/builtin-objects/ecma-builtins.c


      uint32_t bit_for_index = (uint32_t) 1u << index;

      if (!(*bitset_p & bit_for_index) || ecma_op_object_has_own_property (object_p, name_p))
      {
        ecma_append_to_values_collection (for_non_enumerable_p,
-                                          ecma_make_magic_string_value (curr_property_p->magic_string_id),
+                                          ecma_make_magic_string_value (
+                                            (lit_magic_string_id_t) curr_property_p->magic_string_id),


This style is not really preferred, even though vera++ doesn't complain about it. It's not that nice either but breaking out the parameter into a local variable -- i.e., ecma_value_t name = ecma_make_magic_string_value ((lit_magic_string_id_t) curr_property_p->magic_string_id) -- is a more accepted style hereabouts.

This fixes some conversion errors one gets when compiling with LLVM/clang. Most of them are caused when a bit-specific type (i.e. `uint16_t`) is implicitly cast to an `enum` of smaller value range. The fix is just an explicit casting of those types to the desired target type. JerryScript-DCO-1.0-Signed-off-by: Martine Lenders [email protected]

akosthekiss

LGTM

miri64 · 2018-08-16T08:15:08Z

Thanks for the review and analysis of the problem!

miri64 · 2018-08-20T08:45:53Z

Is there anything I still need to do to get this merged?

akosthekiss · 2018-08-20T10:55:27Z

@miri64 Right now, you are clear on your side. Project regulations mandate 2 approvals from maintainers, so one fellow reviewer still needs to take a look at the patch. Once you get that, and assuming that no changes are requested, the PR will be merged.

zherczeg

LGTM

miri64 · 2018-08-21T07:26:45Z

Thank you both for the review!

miri64 mentioned this pull request Aug 15, 2018

jerryscript: Fix conversion errors reported by LLVM/clang RIOT-OS/RIOT#9735

Closed

miri64 force-pushed the fix-clang-conversion-error branch from b7dd5c8 to 66388e3 Compare August 15, 2018 12:01

miri64 force-pushed the fix-clang-conversion-error branch from 66388e3 to 53fa1e2 Compare August 15, 2018 15:53

akosthekiss requested changes Aug 15, 2018

View reviewed changes

miri64 force-pushed the fix-clang-conversion-error branch from 53fa1e2 to 76f03e4 Compare August 15, 2018 16:52

akosthekiss approved these changes Aug 15, 2018

View reviewed changes

akosthekiss mentioned this pull request Aug 17, 2018

Align the RIOT target with the clang build of RIOT OS #2477

Merged

zherczeg approved these changes Aug 21, 2018

View reviewed changes

zherczeg merged commit 851f4f0 into jerryscript-project:master Aug 21, 2018

miri64 deleted the fix-clang-conversion-error branch August 21, 2018 07:26

Fix LLVM/clang conversion errors #2473

Fix LLVM/clang conversion errors #2473

Uh oh!

Conversation

miri64 commented Aug 15, 2018

Uh oh!

akosthekiss commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

akosthekiss commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

akosthekiss commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

akosthekiss commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

akosthekiss commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

akosthekiss commented Aug 15, 2018

Uh oh!

akosthekiss commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

miri64 commented Aug 15, 2018

Uh oh!

akosthekiss Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

miri64 Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 16, 2018

Uh oh!

miri64 commented Aug 20, 2018

Uh oh!

akosthekiss commented Aug 20, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 21, 2018

Uh oh!

Uh oh!