Skip to content

cherry-pick into 1.2: lua: prevent LuaJIT from panicking. (#6994)#83

Merged
duderino merged 1 commit intoistio:release-1.2from
duderino:jblatt_1_2_lua_crash
Jun 12, 2019
Merged

cherry-pick into 1.2: lua: prevent LuaJIT from panicking. (#6994)#83
duderino merged 1 commit intoistio:release-1.2from
duderino:jblatt_1_2_lua_crash

Conversation

@duderino
Copy link

Migration from the build recipes to foreign_cc rules resulted
in dependencies being built with different compiler flags.

Among other things, those compiler flags were added:

-ffunction-sections -fdata-sections

use of which leads to LuaJIT panicking:

PANIC: unprotected error in call to Lua API

and Envoy subsequently crashing.

Broken in envoyproxy#6168.

Fixes istio/istio#13722.

Signed-off-by: Piotr Sikora piotrsikora@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Migration from the build recipes to foreign_cc rules resulted
in dependencies being built with different compiler flags.

Among other things, those compiler flags were added:

    -ffunction-sections -fdata-sections

use of which leads to LuaJIT panicking:

    PANIC: unprotected error in call to Lua API

and Envoy subsequently crashing.

Broken in envoyproxy#6168.

Fixes istio/istio#13722.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link

/lgtm
/approve

+ os.environ["TARGET_CFLAGS"] = os.environ.get("CFLAGS", "")
+ os.environ["TARGET_LDFLAGS"] = os.environ.get("CFLAGS", "")
+ os.environ["TARGET_CFLAGS"] = os.environ.get("CFLAGS", "") + " -fno-function-sections -fno-data-sections"
+ os.environ["TARGET_LDFLAGS"] = os.environ.get("CFLAGS", "") + " -fno-function-sections -fno-data-sections"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there an old cut/paste error carried over?
Why would os.environ["TARGET_LDFLAGS"] not be os.environ.get("LDFLAGS" ...?

Also, isn't LDFLAGS the linker flag and then no need to have these -f that are compiler flags. (aren't linker flags prefixed with -l and -Wl or something?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed this was intentional, but perhaps ask @lizan or on the original PR (envoyproxy#6168)?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looking at LuaJIT's Makefile, it's invoking compiler in place of linker (TARGET_LD= $(CROSS)$(CC)), so I believe that using CFLAGS is correct here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't the override incorrect? LDFLAGS is supposed to override TARGET_LDFLAGS.
c.f. comment from the Makefile
https://github.com/LuaJIT/LuaJIT/blob/v2.1.0-beta3/src/Makefile#L181

@duderino duderino merged commit 082a138 into istio:release-1.2 Jun 12, 2019
rlenglet pushed a commit that referenced this pull request Aug 13, 2019
* Add support for emscripten up to 1.38.39.

Signed-off-by: John Plevyak <jplevyak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants