- 
                Notifications
    
You must be signed in to change notification settings  - Fork 3.4k
 
Don't build MAIN_MODULE as RELOCATABLE #25522
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.
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
Conversation
cd1b8ce    to
    e8f95c7      
    Compare
  
    Also, add an assertion to dynCall. Split out from emscripten-core#25522.
Also, add an assertion to dynCall. Split out from emscripten-core#25522.
8c32591    to
    d40087a      
    Compare
  
    | 
           I still need to get some stats on the benefits of this, but that tests are all now passing so I think this should be good to review now. An open question is: What do we do about   | 
    
Also, add an assertion to dynCall. Split out from emscripten-core#25522.
d40087a    to
    6008112      
    Compare
  
    | 
           Yeah, I see what you mean, removing RELOCATABLE first would simplify this a little. I don't have a preference though. I do see some tests still fail here though?  | 
    
Also, add an assertion to dynCall. Split out from #25522.
637ed91    to
    f56f631      
    Compare
  
    0294236    to
    d499627      
    Compare
  
    | 
           Ok, aside from the problem of what to do with  The stats look really promising. See the new PR description for the great improvements measured when building a dylink version of binaryen.  | 
    
d499627    to
    2c6de78      
    Compare
  
    23d619c    to
    a2e97ec      
    Compare
  
    a2e97ec    to
    4425035      
    Compare
  
    | 
           I solves the   | 
    
c7635d5    to
    a2a8c13      
    Compare
  
    a2a8c13    to
    79e885d      
    Compare
  
    | 
           I think these change should be more of less ready to land now. The old  Once I land this I will likely send a message to mailing list too since its a fairly large change for users of dynamic linking.  | 
    
| if relocatable: | ||
| # Since `-sMAIN_MODULE` no longer implies `-sRELOCATABLE` but we want | ||
| # to keep that cominbation working we run all the `@needs_dylink` tests | ||
| # both with and without the explicit `-sRELOCATABLE` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a lot of slow tests - is it worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is important as long as we want to offer people a fallback using -sRELOCATABLE for some period of time, which I do think is good idea.
Once we remove the fallback option we can then remove all these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much time does it take? I just vaguely worry dylink tests are most of our core time, and doubling them would almost double the total...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two tests total 61 out of 1176 total core2 tests.
In terms of runtime of the core2 test suite they comprise 9 seconds of the full runtime of 63 seconds (I skipped the slow tests there too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sg, thanks for checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comments now and before
5bbdf80    to
    0cdfabc      
    Compare
  
    The main advantage here is that main module no longer requires relocation entries for symbols defined locally. To show the benefits of this approach I build binaryan_wasm with `-sMAIN_MODULE=1` both before and after this change. Before: Wasm Size: 22.6M __wasm_apply_data_relocs size: 123k After: Wasm Size: 16.6M __wasm_apply_data_relocs size: 0 (no longer exists) Fixes: emscripten-core#12682
0cdfabc    to
    55ea688      
    Compare
  
    This issue was introduces in emscripten-core#25522 when `-sRELOCATABLE` was removed from `get_base_cflags. This is a somewhat tricky to reproduce / understand issue with transitive library building: 1. User attempts to compile program using SDL2 + SDL2_GFX. 2. `emcc` attempts to builds all needed libraries in reverse dependency order. And ends up with the follows sequence: 1. libsdl2-pic 2. libsdl2_gfx-pic 3. `emcc` builds libsdl2-pic 4. `emcc` builds libsdl2_gfx-pic. The command that it constructs when building each source file looks like `emcc -fPIC gfx.c -sUSE_SDL=2`. However because neither `-sMAIN_MODULE` nor `-sRELOCATABLE` were specified this `emcc` command will not try to build the non-PIC version of libsdl2. At this point the compiler will crash with: `AssertionError: attempt to lock the cache while a parent process is holding the lock (sysroot/lib/wasm32-emscripten/libSDL2.a)` This issue is hard to reproduce because if libsdl2 is already built it will not trigger the issue.
This issue was introduces in emscripten-core#25522 when `-sRELOCATABLE` was removed from `get_base_cflags. This is a somewhat tricky to reproduce / understand issue with transitive library building: 1. User attempts to compile program using SDL2 + SDL2_GFX. 2. `emcc` attempts to builds all needed libraries in reverse dependency order. And ends up with the follows sequence: 1. libsdl2-pic 2. libsdl2_gfx-pic 3. `emcc` builds libsdl2-pic 4. `emcc` builds libsdl2_gfx-pic. The command that it constructs when building each source file looks like `emcc -fPIC gfx.c -sUSE_SDL=2`. However because neither `-sMAIN_MODULE` nor `-sRELOCATABLE` were specified this `emcc` command will not try to build the non-PIC version of libsdl2. At this point the compiler will crash with: `AssertionError: attempt to lock the cache while a parent process is holding the lock (sysroot/lib/wasm32-emscripten/libSDL2.a)` This issue is hard to reproduce because if libsdl2 is already built it will not trigger the issue.
This issue was introduces in #25522 when `-sRELOCATABLE` was removed from `get_base_cflags. This is a somewhat tricky to reproduce / understand issue with transitive library building: 1. User attempts to compile program using SDL2 + SDL2_GFX. 2. `emcc` attempts to builds all needed libraries in reverse dependency order. And ends up with the follows sequence: 1. libsdl2-pic 2. libsdl2_gfx-pic 3. `emcc` builds libsdl2-pic 4. `emcc` builds libsdl2_gfx-pic. The command that it constructs when building each source file looks like `emcc -fPIC gfx.c -sUSE_SDL=2`. However because neither `-sMAIN_MODULE` nor `-sRELOCATABLE` were specified this `emcc` command will not try to build the non-PIC version of libsdl2. At this point the compiler will crash with: `AssertionError: attempt to lock the cache while a parent process is holding the lock (sysroot/lib/wasm32-emscripten/libSDL2.a)` This issue is hard to reproduce because if libsdl2 is already built it will not trigger the issue.
This means we no longer link main modules with
-pie. The mainadvantage here is that main modules no longer require
runtime relocation code for symbols defined locally.
To show the benefits of this approach I build binaryan_wasm
with
-sMAIN_MODULE=1both before and after this change.Before:
Wasm Size: 22.6M
__wasm_apply_data_relocs size: 123k
After:
Wasm Size: 16.6M
__wasm_apply_data_relocs size: 0 (no longer exists)
Since this is quite a major change, I've kept the
-sRELOCATABLEoption available so folks that use this to get the old behaviour when
linking their main module. In order to tests this path too now run
all the
@needs_dylinktests in test_core.py with and without-sRELOCTABLE. Hopefully we can remove this configurationcompletely at some point in the future.
Fixes: #12682