Isolate GCC specific code #1505

t-harvey · 2016-12-22T00:21:27Z

Folks,
This is the second half of our attempt to enable Jerryscript for compilers other than GCC -- in this case, the TI compilers.

Copied from the commit message:

These changes are designed to enable the TI compilers to compile
Jerryscript.  The changes include:

CMakeLists.txt: we added conditionals around GCC-specific flags, added
support for TI flags, and removed the always-on debugging flag (-g)

/toolchain_mcu_tim4f.cmake: new toolchain file uses TI-specific parameters

jerry-api.h: the sys/types.h file was #include'd but never used, so we
removed it

jrt-types.h: ditto

jerry-port-default-date.c: the TI toolchain doesn't include
sys/time.h, so we guarded uses of the package

...thanks for reviewing these changes!
Tim

robertsipka · 2016-12-22T13:41:00Z

CMakeLists.txt

@@ -88,10 +88,25 @@ macro(jerry_add_compile_flags)
  jerry_add_flags(CMAKE_C_FLAGS ${ARGV})
 endmacro()

+# SHOULD THIS BE, SAY, AT THE TOP OF THE FILE?
+if(CMAKE_C_COMPILER_ID MATCHES "GNU")


I'm not sure that this condition is necessary, maybe you should use CMAKE_COMPILER_IS_GNUCC to check if the compiler is a variant of gcc.

I was working off of the cmake documentation -- https://cmake.org/cmake/help/v3.0/variable/CMAKE_LANG_COMPILER_ID.html -- is there a better/conventional way to do this?

Yes, it's correct in both cases, I was wondering how to avoid this USING_GCC variable, since the CMAKE_COMPILER_IS_GNUCC is also true when CMAKE_C_COMPILER_ID is
"GNU", but I think you're right and using directly the CMAKE_C_COMPILER_ID to determine the GNU compiler is closely aligned with the conventional way.

robertsipka · 2016-12-22T13:41:30Z

CMakeLists.txt

+
+if(CMAKE_C_COMPILER_ID MATCHES "Clang")
+  set(USING_CLANG 1)
+  set(USING_GCC_OR_CLANG 1)


I think the USING_GCC_OR_CLANG variable is unnecessary, instead you should check if the compiler is a variant of gcc or a Clang compiler.

robertsipka · 2016-12-22T13:42:17Z

CMakeLists.txt

@@ -102,22 +117,28 @@ macro(jerry_add_link_flags)
 endmacro()

 # build mode specific compile/link flags
+if(USE_GCC)
 set(CMAKE_C_FLAGS_RELEASE "-Os")


I think you should also specify this optimization level for Clang.

robertsipka · 2016-12-22T13:43:47Z

CMakeLists.txt


 # Architecture-specific compile/link flags
 jerry_add_compile_flags(${FLAGS_COMMON_ARCH})
 jerry_add_flags(CMAKE_EXE_LINKER_FLAGS ${FLAGS_COMMON_ARCH})

 # Enable static build
 if(ENABLE_STATIC_LINK)
-  jerry_add_link_flags("-static")
+  if (USING_GCC_OR_CLANG)


If the static linking isn't supported with TI compilers you should set this value to OFF and send a message that It has been disabled, because the compiler doesn't support it and this condition will be needless.

It's actually the other way: we only support static linking. :-)

This means that we don't have a "-static" flag, which is why it's behind the GCC/Clang conditional...

I see :). If the ENABLE STATIC_LINK is off, please set its value to ON and write a STATUS message that the ENABLE_STATIC_LINK has been enabled.

robertsipka · 2016-12-22T13:44:12Z

CMakeLists.txt

-  jerry_add_compile_flags(-flto)
-  jerry_add_link_flags(-flto)
-  if(CMAKE_COMPILER_IS_GNUCC)
+  if (USING_GCC_OR_CLANG)


You're right -- I missed this one... The TI compiler flag is slightly different, and I've added it, here.

robertsipka · 2016-12-22T13:46:01Z

Thank you for doing this patch, I think this patch is a step in the right direction to build JerryScript with other compilers, like TI compilers.

robertsipka · 2016-12-22T13:49:00Z

CMakeLists.txt


-# Debug information
-jerry_add_compile_flags(-g)


Do you add this flag somewhere else?

I didn't; -- I wasn't confident in this change. :-)

I understand that -g was put in as a default variable b/c of an earlier bug in the code (maybe in jerry-main?...my memory doesn't get better as I age :-) -- and I know that interim fixes have made setting -g as the default unnecessary.

I like the flow that has the user set the cmake variables CMAKE__<DEBUG|RELEASE> to include/exclude settings -- this, I think, matches user expectations...

...but, as with all of these, I'll gladly make an argument and start a discussion, but I've got no ego in this: if I'm wrong here, I'll happily fix it (if we put -g back as an always-set flag, does it go back here, or, if not, where?)...

Seems legit, it's not necessary to compiling a release build with debug flags.

t-harvey · 2016-12-29T18:05:06Z

jerry-core/ecma/operations/ecma-objects-general.c

@@ -313,7 +313,7 @@ ecma_op_general_object_define_own_property (ecma_object_t *object_p, /**< the ob
  JERRY_ASSERT (property_desc_p->is_writable_defined || !property_desc_p->is_writable);

  /* 1. */
-  ecma_extended_property_ref_t ext_property_ref;
+  ecma_extended_property_ref_t ext_property_ref = {0,0};


Somehow, my push caused Travis to report that this variable could be used before being initialized, so I just added the obvious initialization...

t-harvey · 2016-12-29T18:54:58Z

jerry-core/ecma/operations/ecma-objects-general.c

@@ -313,7 +313,7 @@ ecma_op_general_object_define_own_property (ecma_object_t *object_p, /**< the ob
  JERRY_ASSERT (property_desc_p->is_writable_defined || !property_desc_p->is_writable);

  /* 1. */
-  ecma_extended_property_ref_t ext_property_ref = {0,0};
+  ecma_extended_property_ref_t ext_property_ref = {{0},0};


...and now Travis says this is wrong:

./jerry-core/ecma/operations/ecma-objects-general.c:316: error: brace should be placed on a separate line

...I assume that this is a style checker, objecting to the braces? This code, however, looks like the right way to write it: am I not following a standard, or is there a bug in the style-checking code?

zherczeg · 2017-01-05T13:03:38Z

Please rebase this patch to master.

t-harvey · 2017-01-05T23:56:19Z

Robert,
In re ENABLE_STATIC_LINK -- so I played with a number of error messages, and I never got one I liked: they either fell in unexpected places (so the user isn't looking for them), or they interrupted other output (they just got in the way).

I compromised by hanging a comment off of the current printout of flags:

...normally, you'd see this:

-- ENABLE_ALL_IN_ONE OFF
-- ENABLE_LTO ON
-- ENABLE_STATIC_LINK ON
-- ENABLE_STRIP ON
-- JERRY_CMDLINE OFF

...if the compiler doesn't do dynamic linking, you get this:

-- ENABLE_ALL_IN_ONE OFF
-- ENABLE_LTO ON
-- ENABLE_STATIC_LINK ON (ONLY OPTION FOR THIS COMPILER)
-- ENABLE_STRIP ON
-- JERRY_CMDLINE OFF

...is this okay?

I've squashed all of the commits, so if everything looks good, this is ready for pulling.

zherczeg · 2017-01-06T04:28:25Z

The #1483 should be landed first since this PR depends on that.

robertsipka · 2017-01-06T08:52:22Z

@t-harvey : Yes, this message form is perfect, thanks.
I would not set the USING_GCC_OR_CLANG variable. Could you use "USING_GCC OR USING_CLANG" in the affected conditions instead of "USING_GCC_OR_CLANG"? LGTM (informally) after these changes.

LaszloLango · 2017-01-09T08:43:40Z

@t-harvey, please rebase to the current master.

robertsipka · 2017-01-09T21:17:12Z

CMakeLists.txt

+if(USING_GCC OR USING_CLANG)
+set(CMAKE_C_FLAGS_RELEASE "-Os")
+endif()
+


This is not needed anymore, see #1513.

Removed... I had a little trouble getting the rebase done (and I had a failure in the runtime tests, which was weird, as it went away over the course of the afternoon :-), but it should be ready to go now...

LaszloLango · 2017-01-10T08:04:05Z

cmake/toolchain_mcu_tim4f.cmake

+SET (CMAKE_CXX_FLAGS_DEBUG_INIT          "-g")
+SET (CMAKE_CXX_FLAGS_MINSIZEREL_INIT     "-o4 -mf0 -DNDEBUG")
+SET (CMAKE_CXX_FLAGS_RELEASE_INIT        "-o4 -DNDEBUG")
+SET (CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT "-o2 -g")


Please add new line at the end of the file

LaszloLango · 2017-01-10T08:21:01Z

jerry-core/ecma/operations/ecma-objects-general.c

@@ -313,7 +313,7 @@ ecma_op_general_object_define_own_property (ecma_object_t *object_p, /**< the ob
  JERRY_ASSERT (property_desc_p->is_writable_defined || !property_desc_p->is_writable);

  /* 1. */
-  ecma_extended_property_ref_t ext_property_ref;
+  ecma_extended_property_ref_t ext_property_ref = {{0},0};


The Vera++ rule is not clever enough to recognize this. But the correct style would be { { 0 }, 0 }
I suggest to use a define to workaround this problem.

diff --git a/jerry-core/ecma/base/ecma-globals.h b/jerry-core/ecma/base/ecma-globals.h index 99b6da3..fa87e91 100644 --- a/jerry-core/ecma/base/ecma-globals.h +++ b/jerry-core/ecma/base/ecma-globals.h @@ -468,6 +468,11 @@ typedef struct #define ECMA_PROPERTY_SEARCH_DEPTH_LIMIT 128 /** + * Empty extended property reference + */ +#define ECMA_EMPTY_EXTENDED_PROPERTY_REFERENCE { { 0 }, 0 } + +/** * Property reference. It contains the value pointer * for real, and the value itself for virtual properties. */ diff --git a/jerry-core/ecma/operations/ecma-objects-general.c b/jerry-core/ecma/operations/ecma-objects-general.c index c24f356..5b479b2 100644 --- a/jerry-core/ecma/operations/ecma-objects-general.c +++ b/jerry-core/ecma/operations/ecma-objects-general.c @@ -313,7 +313,7 @@ ecma_op_general_object_define_own_property (ecma_object_t *object_p, /**< the ob JERRY_ASSERT (property_desc_p->is_writable_defined || !property_desc_p->is_writable); /* 1. */ - ecma_extended_property_ref_t ext_property_ref; + ecma_extended_property_ref_t ext_property_ref = ECMA_EMPTY_EXTENDED_PROPERTY_REFERENCE; ecma_property_t current_prop; current_prop = ecma_op_object_get_own_property (object_p,

LaszloLango

LGTM

zherczeg · 2017-01-13T12:05:15Z

The #1483 should be landed first since this PR depends on that.

Just to not forget about it.

tilmannOSG

LGTM with minor style nitpicks :)
Thanks for working on this! Great to see that JerryScript can now be built for TI devices :)

tilmannOSG · 2017-01-13T12:46:13Z

CMakeLists.txt

+
+if(USING_TI)
+# if using a compiler that _only_ does static linking, inform the user
+# of the discrepancy in settings


Please start the comment with an upper case letter and end with a full stop :)

tilmannOSG · 2017-01-13T12:49:39Z

jerry-core/ecma/operations/ecma-objects-general.c

@@ -313,7 +313,9 @@ ecma_op_general_object_define_own_property (ecma_object_t *object_p, /**< the ob
  JERRY_ASSERT (property_desc_p->is_writable_defined || !property_desc_p->is_writable);

  /* 1. */
-  ecma_extended_property_ref_t ext_property_ref;
+  /* this #def just gets around the syntax/style checker...*/


Please start the comment with an upper case letter as well :)

tilmannOSG · 2017-01-17T07:51:02Z

@t-harvey Thanks for the update, this PR is now ready to be merged (can be merged once #1483 has landed).

LaszloLango · 2017-01-23T11:28:10Z

@t-harvey, #1483 is merged. Please rebase, so we can merge this PR too.

Jerryscript. The changes include: CMakeLists.txt: we added conditionals around GCC-specific flags, added support for TI flags, and removed the always-on debugging flag (-g) /toolchain_mcu_tim4f.cmake: new toolchain file uses TI-specific parameters jerry-api.h: the sys/types.h file was #include'd but never used, so we removed it jrt-types.h: ditto jerry-port-default-date.c: the TI toolchain doesn't include sys/time.h, so we guarded uses of the package ecma-objects-general.c: added initialization that Travis (the auto-checking tool) required JerryScript-DCO-1.0-Signed-off-by: Timothy Harvey [email protected]

PR jerryscript-project#1505 added support for TI compiler. It explicitly added a message to notify the user that static linking is forced. PR jerryscript-project#1755 added a more generic approach to signal such forced settings and adapted the TI-specific static linking notification to this approach. However, it turned out that TI forcibly changed another setting, too: it disabled release binary stripping, but without notification. This patch fixes this by moving the setting override to a consistent place and adding a notification. PR jerryscript-project#1505 also added some source code changes, most importantly a complex struct initialization for a variable in `ecma-objects-general.c`. However, that initialization was coded as a macro to trick the style checker. This patch gets rid of that macro and uses proper C99 struct initializer with designators. JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

PR #1505 added support for TI compiler. It explicitly added a message to notify the user that static linking is forced. PR #1755 added a more generic approach to signal such forced settings and adapted the TI-specific static linking notification to this approach. However, it turned out that TI forcibly changed another setting, too: it disabled release binary stripping, but without notification. This patch fixes this by moving the setting override to a consistent place and adding a notification. PR #1505 also added some source code changes, most importantly a complex struct initialization for a variable in `ecma-objects-general.c`. However, that initialization was coded as a macro to trick the style checker. This patch gets rid of that macro and uses proper C99 struct initializer with designators. JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

robertsipka reviewed Dec 22, 2016

View reviewed changes

t-harvey commented Dec 29, 2016

View reviewed changes

t-harvey force-pushed the isolate_gcc_specific_code branch from 46c792d to 8b8f34c Compare January 4, 2017 16:33

t-harvey force-pushed the isolate_gcc_specific_code branch 2 times, most recently from 0c99599 to 98b3700 Compare January 5, 2017 23:46

t-harvey force-pushed the isolate_gcc_specific_code branch from 98b3700 to 2b68446 Compare January 6, 2017 14:59

t-harvey force-pushed the isolate_gcc_specific_code branch from 2b68446 to 8c00f15 Compare January 9, 2017 20:05

robertsipka reviewed Jan 9, 2017

View reviewed changes

t-harvey force-pushed the isolate_gcc_specific_code branch from 8c00f15 to d1de4e8 Compare January 9, 2017 21:36

LaszloLango requested changes Jan 10, 2017

View reviewed changes

t-harvey force-pushed the isolate_gcc_specific_code branch 2 times, most recently from 839e91f to 592ebc3 Compare January 10, 2017 15:33

LaszloLango approved these changes Jan 13, 2017

View reviewed changes

tilmannOSG changed the title ~~Isolate gcc specific code~~ Isolate GCC specific code Jan 13, 2017

tilmannOSG approved these changes Jan 13, 2017

View reviewed changes

t-harvey force-pushed the isolate_gcc_specific_code branch from 592ebc3 to 502cdf0 Compare January 13, 2017 21:56

t-harvey force-pushed the isolate_gcc_specific_code branch from 5ad0ea7 to ab46d13 Compare January 23, 2017 20:09

tilmannOSG merged commit f88d1a4 into jerryscript-project:master Jan 24, 2017

akosthekiss mentioned this pull request Apr 22, 2017

Aftermath of PRs #1505 and #1755 #1771

Merged

zherczeg mentioned this pull request Mar 27, 2018

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

Closed

Isolate GCC specific code #1505

Isolate GCC specific code #1505

Uh oh!

Conversation

t-harvey commented Dec 22, 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 Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 22, 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!

zherczeg commented Jan 5, 2017

Uh oh!

t-harvey commented Jan 5, 2017

Uh oh!

zherczeg commented Jan 6, 2017

Uh oh!

robertsipka commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaszloLango commented Jan 9, 2017

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!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tilmannOSG commented Jan 17, 2017

Uh oh!

LaszloLango commented Jan 23, 2017

Uh oh!

Uh oh!

robertsipka Dec 22, 2016 •

edited

Loading

robertsipka commented Jan 6, 2017 •

edited

Loading

zherczeg commented Jan 13, 2017 •

edited

Loading