Add #if control around GCC builtin functions so that the code can be compiled using non-GCC compilers. #1483

t-harvey · 2016-12-10T00:32:46Z

The header says it all -- only some of the GCC builtin functions are protected by "#ifdef (GNUC)", and this change would protect the builtin functions in jrt.h and assert.h.

robertsipka · 2016-12-10T11:26:10Z

jerry-core/jrt/jrt.h

@@ -1,4 +1,5 @@
-/* Copyright JS Foundation and other contributors, http://js.foundation
+/* Copyright 2014-2016 Samsung Electronics Co., Ltd.
+ * Copyright 2016 University of Szeged.
 *


Please don't change the copyrights.

robertsipka · 2016-12-10T11:26:46Z

jerry-core/jrt/jrt.h

@@ -43,6 +44,10 @@
 /*
 * Conditions' likeliness, unlikeliness.
 */
+#ifndef (__GNUC__)


Don't enclose the proceeding identifier with brackets.

robertsipka · 2016-12-10T11:28:51Z

jerry-core/jrt/jrt.h

@@ -43,6 +44,10 @@
 /*
 * Conditions' likeliness, unlikeliness.
 */
+#ifndef (__GNUC__)
+#define __builtin_expect(expression, expected_value) (expression)


I think you should wrap the defines in an #ifdef to ensure compilation on other compilers,
instead of define the __builtin_expect(). Like this.

#ifdef __GNUC__ #define likely(x) __builtin_expect(!!(x), 1) #define unlikely(x) __builtin_expect(!!(x), 0) #else /* ! __GNUC__ */ #define likely(x) (x) #define unlikely(x) (x) #endif /* __GNUC__ */

No space after !

I agree, turning the macros into a "no op" is preferred over redefining the (non-existant) builtins.

robertsipka · 2016-12-10T11:34:33Z

Please fix the DCO in the commit message. Check travis log for details:
https://travis-ci.org/jerryscript-project/jerryscript/jobs/182762008

zherczeg · 2016-12-12T07:43:52Z

jerry-core/jrt/jrt.h

 #define JERRY_UNREACHABLE() __builtin_unreachable ()
+#else /* !__GNUC__ */
+#define JERRY_UNREACHABLE() abort(); /* but what if they haven't included stdlib?!? */


Perhaps we should change it to JERRY_ASSERT (0);

@zherczeg The problem is that it would change the semantics of JERRY_UNREACHABLE() in release builds since JERRY_ASSERT() becomes a "no op" when JerryScript is built in release mode. E.g. today __builtin_unreachable() is called when JERRY_UNREACHABLE() is hit in a release build.
Adding abort() and an include directive for stdlib.h should be fine as this yields the intended behavior no matter which compiler is used.

No, this isn't exactly working that way. __builtin_unreachable is NOT a function call, it just notifies the compiler, that the code after that is unreachable, and it can optimize the code that way. The compiler is very serious about it even in debug mode, so we needed to add an assert to capture such issues instead of random crashes.

Hm, perhaps the best would be an empty macro in relase, and an ASSERT in debug. Perhaps the ASSERT is added automatically in debug.

I think there's some misunderstanding here :)
__builtin_unreachable() is a built-in function which causes undefined behavior when called during actual execution. Since it's a function that never returns the compiler can make assumptions based on that (but so can the compiler about abort() which also never returns). The current behavior of JERRY_UNREACHABLE() works just fine in debug builds with a non-GCC compiler so we don't need to worry about that. However, for release builds we need an alternative to __builtin_unreachable() since this is a GCC-specific function.
abort() provides us exactly that: It terminates the program in a controlled manner (rather than just crashing the program) and since abort() is part of the C standard library this will work just fine with any C99-compliant compiler.
Makes sense? :)
(BTW assert() is typically implemented on top of abort() e.g. it prints some details about the location of the assertion in the code and then just calls abort())

One thing I forgot to mention: The key difference between __builtin_unreachable() and abort() is that calling __builtin_unreachable() yields undefined behavior while calling abort() is perfectly valid, e.g. not triggering any kind of undefined behavior. The compiler obviously can optimize much more aggressively when undefined behavior is involved. However, since the goal here is to have a solution that works in a standard C99 environment without relying on any compiler-specific built-in functions, abort() seems to be the closest match to __builtin_unreachable() in terms of intended behavior. Losing some optimization opportunities is essentially the price that needs to be paid to get a solution that works in any standard C99 environment.

I have some memories about __builtin_unreachable(), when a bad code actually reached this point, and no return code was generated by the GCC. Hence the code entered into the next function. Hard to debug. This was the reason while we added an assert before __builtin_unreachable(), so we can capture these issues in debug mode.

Yes, GCC can do that since hitting __builtin_unreachable() triggers undefined behavior :)

zherczeg · 2016-12-12T07:44:41Z

jerry-libc/include/assert.h

+
+#ifndef (__GNUC__)
+#define __builtin_expect(expression, expected_value) (expression)
+#endif /* !__GNUC__ */


We could remove this if we follow @robertsipka suggestion.

Yes, this should be removed :)

tilmannOSG

Thanks for working on this! Making it easy to build JerryScript with all the different compilers out there is very important for the project.
For more information about the DCO please also have a look at our Contribution Guidelines: https://github.com/jerryscript-project/jerryscript/blob/master/CONTRIBUTING.md

tilmannOSG · 2016-12-12T08:51:17Z

jerry-core/jrt/jrt.h

@@ -43,6 +44,10 @@
 /*
 * Conditions' likeliness, unlikeliness.
 */
+#ifndef (__GNUC__)
+#define __builtin_expect(expression, expected_value) (expression)


I agree, turning the macros into a "no op" is preferred over redefining the (non-existant) builtins.

tilmannOSG · 2016-12-12T08:58:59Z

jerry-core/jrt/jrt.h

 #define JERRY_UNREACHABLE() __builtin_unreachable ()
+#else /* !__GNUC__ */
+#define JERRY_UNREACHABLE() abort(); /* but what if they haven't included stdlib?!? */


@zherczeg The problem is that it would change the semantics of JERRY_UNREACHABLE() in release builds since JERRY_ASSERT() becomes a "no op" when JerryScript is built in release mode. E.g. today __builtin_unreachable() is called when JERRY_UNREACHABLE() is hit in a release build.
Adding abort() and an include directive for stdlib.h should be fine as this yields the intended behavior no matter which compiler is used.

tilmannOSG · 2016-12-12T08:59:38Z

jerry-libc/include/assert.h

+
+#ifndef (__GNUC__)
+#define __builtin_expect(expression, expected_value) (expression)
+#endif /* !__GNUC__ */


Yes, this should be removed :)

zherczeg · 2016-12-16T09:18:20Z

jerry-core/jrt/jrt.h

+#else /* !__GNUC__ */
+#define JERRY_UNREACHABLE() { \
+#include <stdlib.h> \
+abort(); \


The question still stands: do we need to do anything here?

extern void __noreturn jerry_unreachable (const char *, const char *, const uint32_t);

In debug mode, the unreachable function calls a real function, which yields an error and terminates the process. In release mode the undefinied behaviour is expected. Hence why we just do nothing here? Why do we increase the binary size by adding abort calls here?

Yes I agree, I think based on our previous discussion in this PR JERRY_UNREACHABLE() can become a "no op" when built with a non-GCC compiler. __builtin_unreachable() is essentially just a hint to the compiler that the code can never be reached.

tilmannOSG · 2016-12-16T10:10:25Z

@t-harvey Can you please change JERRY_UNREACHABLE() to become a "no op" and squash all your changes into a single commit? I think after that the PR is ready to be merged :)

robertsipka · 2017-01-04T13:29:13Z

jerry-libc/include/assert.h

+
+#ifndef __GNUC__
+#define __builtin_expect(expression, expected_value) (expression)
+#endif /* !__GNUC__ */


As I see it this is the main reason why this patch hasn't been merge yet. The assert function uses __builtin_expect (below in a condition).
@zherczeg , @tilmannOSG : Should this be remained in this form, or should the assert function be defined differently, or do you have any other ideas?

Guys -- I thought the only reason this hadn't yet been merged was b/c I forgot to ping Tillman that I had added the DCO message (sorry, Tillman! :-) -- what am I missing/what can I do to fix this other problem? (I had assumed that if the code passed Travis that it was good to go...?)

Ohh, I missed Tilmann's comment, sorry, this define has been approved earlier.

I would prefer to replace the __builtin_expect() to unlikely() here. The project should not rely on __builtin_expect.

I agree with @zherczeg, since the only direct use of __builtin_expect() is in assert.h this is best fixed by just replacing this invocation with unlikely().

@tilmannOSG && @zherczeg -- I'd like to propose a different way of looking at this:

The GCC constructs are useful (to GCC :-); removing them seems like we're getting rid of functionality. Since, with this #def, they behave just fine for every other compiler, why not leave them?

Secondly, "unlikely" is only defined in jrt.h; using it all over the place forces (read: makes it a really good idea :-) us to reorganize things in .h files (for this change, because now we suddenly have to include jrt.h in assert.h, which makes no obvious sense) -- these kinds of reorganizations are required maintenance for an evolving project, but kind of out of the scope for this otherwise simple change (since the bigger fix requires system architectural knowledge, discussion of division, what/when/where to include what, etc... :-).

...so for the record: I'm not trying to get out of work by suggesting that we leave this code as written :-)...but doing this correctly will take a lot of work (and reduce functionality!), so I am actually saying that we should check this in as is...

@t-harvey Good point, I think in this case the pragmatic solution is to just drop the __builtin_expect() use from assert.h. Since this is only affects the code in jerry-libc (rather than jerry-core) and most of the assert() calls seem to be in the printf() implementation (which is not really performance-critical code) the performance impact should be negligible. Builds with asserts enabled are expected to have a noticeable performance overhead anyway.

zherczeg · 2017-01-05T12:59:14Z

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

@@ -343,7 +343,7 @@ ecma_builtin_global_object_parse_int (ecma_value_t this_arg, /**< this argument
        while (string_curr_p > start_p)
        {
          ecma_char_t current_char = *(--string_curr_p);
-          ecma_number_t current_number;
+          ecma_number_t current_number = ECMA_NUMBER_MINUS_ONE;

          if ((current_char >= LIT_CHAR_LOWERCASE_A && current_char <= LIT_CHAR_LOWERCASE_Z))
          {


I would simply add an assert around lit_char_is_decimal_digit (current_char).

@zherczeg While that might work to eliminate the compiler warning, it seems explicitly initializing the variable is still the better option (we should initialize all variables by default to avoid any bugs due to uninitialized variables).

zherczeg · 2017-01-05T13:02:26Z

jerry-libc/include/assert.h

+
+#ifndef __GNUC__
+#define __builtin_expect(expression, expected_value) (expression)
+#endif /* !__GNUC__ */


I would prefer to replace the __builtin_expect() to unlikely() here. The project should not rely on __builtin_expect.

tilmannOSG · 2017-01-13T12:22:45Z

@t-harvey If you just replace the __builtin_expect() call in assert.h with a call to unlikely() then we can go ahead and merger this pull request :)

zherczeg · 2017-01-16T12:50:17Z

Ok. I haven't noticed we are talking about libc. Yes, just drop it.

tilmannOSG · 2017-01-16T12:55:16Z

@t-harvey So if you just remove the __builtin_expect() call from assert.h (and the __builtin_expect macro in the current patch) then we're good to go to get this finally merged :)

zherczeg

LGTM after the endif fixed.

zherczeg · 2017-01-20T08:51:07Z

jerry-core/jrt/jrt.h

 #define JERRY_UNREACHABLE() __builtin_unreachable ()
+#else /* !__GNUC__ */
+#define JERRY_UNREACHABLE()
+#endif /* !__GNUC__ */


#endif /* __GNUC__ */

compiled using non-GCC compilers. This included adding an initialization to a variable that looks(!) like it can reach a use before being initialized (b/c we turned JERRY_UNREACHABLE into a nop) -- an example: int foo; switch (value_can_only_be_one_or_two) case 1: case 2: foo = 5; default: JERRY_UNREACHABLE(); x = foo +1; ...the compiler assumes that the path can go through the default case, which leaves the value of foo undefined. JerryScript-DCO-1.0-Signed-off-by: Timothy Harvey [email protected]

robertsipka reviewed Dec 10, 2016

View reviewed changes

zherczeg reviewed Dec 12, 2016

View reviewed changes

tilmannOSG requested changes Dec 12, 2016

View reviewed changes

zherczeg reviewed Dec 16, 2016

View reviewed changes

t-harvey force-pushed the fix_gcc_builtins branch 3 times, most recently from 8abf301 to bc3d01a Compare December 20, 2016 21:54

robertsipka reviewed Jan 4, 2017

View reviewed changes

zherczeg requested changes Jan 5, 2017

View reviewed changes

zherczeg mentioned this pull request Jan 6, 2017

Isolate GCC specific code #1505

Merged

t-harvey force-pushed the fix_gcc_builtins branch 2 times, most recently from a4bd63a to 9885d07 Compare January 17, 2017 17:17

zherczeg approved these changes Jan 20, 2017

View reviewed changes

t-harvey force-pushed the fix_gcc_builtins branch from 9885d07 to ede4604 Compare January 20, 2017 19:39

tilmannOSG approved these changes Jan 23, 2017

View reviewed changes

tilmannOSG merged commit 94b6aae into jerryscript-project:master Jan 23, 2017

zherczeg mentioned this pull request Mar 27, 2018

Nominating Robert Sipka (robertsipka) to become a reviewer #2254

Closed

Add #if control around GCC builtin functions so that the code can be compiled using non-GCC compilers. #1483

Add #if control around GCC builtin functions so that the code can be compiled using non-GCC compilers. #1483

Uh oh!

Conversation

t-harvey commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsipka commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tilmannOSG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tilmannOSG commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tilmannOSG commented Jan 13, 2017

Uh oh!

zherczeg commented Jan 16, 2017

Uh oh!

tilmannOSG commented Jan 16, 2017

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!