Skip to content

Don't use whole-archive unless actaully needed - #11061

Closed
sbc100 wants to merge 1 commit into
masterfrom
include_libs
Closed

Don't use whole-archive unless actaully needed#11061
sbc100 wants to merge 1 commit into
masterfrom
include_libs

Conversation

@sbc100

@sbc100 sbc100 commented May 1, 2020

Copy link
Copy Markdown
Collaborator

Sometime emcc wants to add extra libraries the link, but when it
does do there is no need to use --whole-archive which is what is
used for FORCE_STDLIBS

This change was split out from:
#10998

@sbc100
sbc100 requested a review from kripken May 1, 2020 23:49
sbc100
sbc100 commented May 1, 2020
View reviewed changes
Comment thread tests/other/metadce/hello_libcxx_O2_fexceptions_DEMANGLE_SUPPORT.funcs
$pop_arg
$pop_arg_long_double
$printf_core
$pthread_cond_wait

@sbc100 sbc100 May 1, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This seems to be the result of no using --whole-archive around libc++abi.. I'm not sure what to make of it .. but I'm pretty sure we don't want --whole-archive

@kripken kripken May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if it happens to cause a different order of functions, and then different function sizes (LEBs of call indexes change), and it's just enough to cause an inlining difference...

Maybe just verify locally that the wasm size hasn't changed much? (the metadce test should, but would be good to know if this is just under the size of the slack we allow to change)

kripken
kripken approved these changes May 2, 2020
View reviewed changes

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the behavior of EMCC_ONLY_FORCED_STDLIBS (line 1482) change with this?

Comment thread emcc.py Outdated
shared.Settings.MAX_WEBGL_VERSION = 2

forced_stdlibs = []
include_stdlibs = []

@kripken kripken May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

included, with a d perhaps? at least for me, include_stdlibs sounds like a boolean, not a list of included stdlibs.

Comment thread tests/other/metadce/hello_libcxx_O2_fexceptions_DEMANGLE_SUPPORT.funcs
$pop_arg
$pop_arg_long_double
$printf_core
$pthread_cond_wait

@kripken kripken May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if it happens to cause a different order of functions, and then different function sizes (LEBs of call indexes change), and it's just enough to cause an inlining difference...

Maybe just verify locally that the wasm size hasn't changed much? (the metadce test should, but would be good to know if this is just under the size of the slack we allow to change)

Comment thread tools/system_libs.py Outdated
@sbc100

sbc100 commented May 2, 2020

Copy link
Copy Markdown
Collaborator Author

I think EMCC_FORCED_STDLIBS and EMCC_ONLY_FORCED_STDLIBS should both remain unchanged here.. at least that is my intention.

sbc100 added a commit that referenced this pull request May 2, 2020
@sbc100
No need for force all of libc++abi just to get __cxa_demangle
6d13e51 
These two lines were both added back in:
812f051

However, simply depending on the symbol should be enough to link it
in (and generate and error if its missing).

Split out from #11061
sbc100 added a commit that referenced this pull request May 2, 2020
@sbc100
No need for force all of libc++abi just to get __cxa_demangle
9f89c70 
These two lines were both added back in:
812f051

However, simply depending on the symbol should be enough to link it
in (and generate and error if its missing).

Split out from #11061
@sbc100 sbc100 mentioned this pull request May 2, 2020
Merged
@sbc100

sbc100 commented May 2, 2020

Copy link
Copy Markdown
Collaborator Author

I might just land #11063 instead since libc++abi is the only lib here is many different object files in it.

sbc100 added a commit that referenced this pull request May 2, 2020
@sbc100
No need for force all of libc++abi just to get __cxa_demangle
b1a570f 
These two lines were both added back in:
812f051

However, simply depending on the symbol should be enough to link it
in (and generate and error if its missing).

Split out from #11061
sbc100 added a commit that referenced this pull request May 2, 2020
@sbc100
No need for force all of libc++abi just to get __cxa_demangle
fcaed02 
These two lines were both added back in:
812f051

However, simply depending on the symbol should be enough to link it
in (and generate and error if its missing).

Split out from #11061
sbc100 added a commit that referenced this pull request May 3, 2020
@sbc100
No need for force all of libc++abi just to get __cxa_demangle (#11063)
962176e 
These two lines were both added back in:
812f051

However, simply depending on the symbol should be enough to link it
in (and generate and error if its missing).

Split out from #11061
@sbc100
Don't use whole-archive unless actaully needed
024c940 
Sometime emcc wants to add extra libraries the link, but when it
does do there is no need to use `--whole-archive` which is what is
used for FORCE_STDLIBS

This change was split out from:
#10998
@sbc100 sbc100 closed this May 12, 2020
@sbc100
sbc100 deleted the include_libs branch May 12, 2020 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kripken kripken kripken approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sbc100 @kripken