Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle -Wl,--stack-first being passed explicitly #21216

Merged
merged 1 commit into from
Jan 30, 2024
+24 −9
Merged

Handle -Wl,--stack-first being passed explicitly #21216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1197,6 +1197,15 @@ def test_wl_linkflags(self):
self.run_process([EMCC, 'main.c', '-Wl,-L.', '-Wl,-lfoo'])
self.run_process([EMCC, 'main.c', '-Wl,@linkflags.txt'])

def test_wl_stackfirst(self):
cmd = [EMCC, test_file('hello_world.c'), '-Wl,--stack-first']
self.run_process(cmd + ['-O0'])
self.run_process(cmd + ['-O2'])
err = self.expect_fail(cmd + ['-fsanitize=address'])
self.assertContained('error: --stack-first is not compatible with asan', err)
err = self.expect_fail(cmd + ['-sGLOBAL_BASE=1024'])
self.assertContained('error: --stack-first is not compatible with -sGLOBAL_BASE', err)

def test_l_link(self):
# Linking with -lLIBNAME and -L/DIRNAME should work, also should work with spaces
create_file('main.c', '''
Expand Down
24 changes: 15 additions & 9 deletions tools/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,14 +879,6 @@ def phase_linker_setup(options, state, newargs):
else:
default_setting('INCOMING_MODULE_JS_API', [])

if 'GLOBAL_BASE' not in user_settings and not settings.SHRINK_LEVEL and not settings.OPT_LEVEL:
# When optimizing for size it helps to put static data first before
# the stack (since this makes instructions for accessing this data
# use a smaller LEB encoding).
# However, for debugability is better to have the stack come first
# (because stack overflows will trap rather than corrupting data).
settings.STACK_FIRST = True

# Default to TEXTDECODER=2 (always use TextDecoder to decode UTF-8 strings)
# in -Oz builds, since custom decoder for UTF-8 takes up space.
# In pthreads enabled builds, TEXTDECODER==2 may not work, see
Expand Down Expand Up @@ -1592,7 +1584,6 @@ def check_memory_setting(setting):
# We start our global data after the shadow memory.
# We don't need to worry about alignment here. wasm-ld will take care of that.
settings.GLOBAL_BASE = shadow_size
settings.STACK_FIRST = False

if not settings.ALLOW_MEMORY_GROWTH:
settings.INITIAL_MEMORY = total_mem
Expand All @@ -1615,6 +1606,21 @@ def check_memory_setting(setting):
if sanitize and settings.GENERATE_SOURCE_MAP:
settings.LOAD_SOURCE_MAP = 1

if 'GLOBAL_BASE' not in user_settings and not settings.SHRINK_LEVEL and not settings.OPT_LEVEL and not settings.USE_ASAN:
# When optimizing for size it helps to put static data first before
# the stack (since this makes instructions for accessing this data
# use a smaller LEB encoding).
# However, for debugability is better to have the stack come first
# (because stack overflows will trap rather than corrupting data).
settings.STACK_FIRST = True

if '--stack-first' in [x for _, x in state.link_flags]:
settings.STACK_FIRST = True
Comment on lines +1617 to +1618
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, you can run into lots of (sometimes subtle) trouble with -Wl,XYZ where an -sXYZ setting also exists and interacts in some way...

So I am just curious where has the line in the sand been drawn for such things?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that we cannot support all possible -Wl,xx and -s settings. I don't have strong opinions on how much effort we should put into supporting the combinations that we can.

One alternative version of this PR would simply to be to error out if the user passes -Wl,--stack-first. Another would be to do nothing and continue to fail in way that less actionable.

I think I'm OK with this best effort level of support we that we seem to have have today.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT?

I suppose if I were designing emcc from zero, I would avoid introducing sXYZs where they would in any way overlap with linker settings, and error in cases where the overlap is inevitable. That's not how emcc is designed though - it's very permissive, which is also a very valid choice given its goal of emulating existing linkers as best as it can.

Given this, it seems as fine a strategy as any to do things on an as-users-have-reported-this basis. As you note, it will never be complete. E. g. in this change:

  1. It seems that explicit -sSTACK_FIRST=1 does not lead to an error?
  2. -Wl,--stack-first overrides explicit -sSTACK_FIRST=0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-sSTACK_FIRST is not a thing. The STACK_FIRST setting is an internal setting, not settable by the user.

We do try hard to avoid -s settings that overlap with the linker/compiler BTW. We've doing that several times in the past and will continue to to that going forward. Using clang / lld standard flags to always preferable IMHO.

Copy link
Contributor

@SingleAccretion SingleAccretion Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

👍.

-sSTACK_FIRST is not a thing

That's an oversight by me, sorry about that.

Using clang / lld standard flags to always preferable IMHO.

So that opens the question whether I should rewrite #21071 to use -Wl,--initial-heap=...... There is no place to put the documentation, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a good point. Those memory settings, along with -sGOBAL_BASE is areas where we do currently override and replace the linker default settings.

I think I'm ok with adding -sINITIAL_MEMORY for now and then attempting to migrate users to native linker flags later, since there are a bunch of different memory related linker flags today that fall in the same bucket: INITIAL_MEMORY, MAXIUMUM_MEMORY, GLOBAL_BASE, STACK_SIZE, etc..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For background I think one of the reasons these settings evolves the way they did is the emscripten was original written before wasm-ld existed. It implemented the linker itself back then, with its own flags.

if settings.USE_ASAN:
exit_with_error('--stack-first is not compatible with asan')
if 'GLOBAL_BASE' in user_settings:
exit_with_error('--stack-first is not compatible with -sGLOBAL_BASE')

set_max_memory()

# check if we can address the 2GB mark and higher.
Expand Down
Loading