Always use -fmacro-prefix-map #23212
Conversation
When `deterministic_paths` is set, we are currently using `-ffile-prefix-map` to produce the same path in data and debug info. In the case of absolute paths, their emscripten path is replaced with a fake path `/emsdk/emscripten`, and in the case of relative paths, all path relative to the emscripten directory is removed, so `../../system/lib/somefile.c` becomes `system/lib/somefiles.c`. https://github.com/emscripten-core/emscripten/blob/f66b5d706e174d9e5cc6122c06ea29dcd2735cd0/tools/system_libs.py#L472-L477 https://github.com/emscripten-core/emscripten/blob/f66b5d706e174d9e5cc6122c06ea29dcd2735cd0/tools/system_libs.py#L495-L501 But this does not make relative paths and absolute paths the same, which can be a problem when data generated by `__FILE__` macro is included in one of code size tests. This problem is discussed in emscripten-core#23195. This PR makes `__FILE__` macro produce the same data in all cases by using the fake path `/emsdk/emscripten` as its base, so that it wouldn't change any results for code size tests. This is done by `-fmacro-prefix-map`. This differs from the current behavior because we don't handle relative and absolute paths differently. For the debug info, when `deterministic_paths` is set, this uses a fake path `/emsdk/emscripten` as a base emscripten path. When `deterministic_paths` is not set, this uses real local absolute paths in the debug info. This allows local developers to see their real paths in the debug info while continuing to use the same (fake) path `/emsdk/emscripten` we have used so far for the release binaries. Users can set their debug base path to whatever path they like, but given that we have used `/emsdk/emscripten` in release binaries for a while, it is possible that some users have set their configuration with this directory, so it would be better not to break them by changing it. This is done by `-ffile-prefix-map` as we have done so far, which is an alias for both `-fdebug-prefix-map` and `-fmacro-prefix-map`. This is basically implementing what's suggested in emscripten-core#23195 (comment) and emscripten-core#23195 (comment) This also turns `deterministic_paths` on for the Ninja path in embuilder for consistency with the non-Ninja path. Fixes #23915.
tools/system_libs.py
Outdated
| cflags += [f'-ffile-prefix-map={relative_source_dir}/='] | ||
| cflags += [f'-ffile-prefix-map={source_dir}=/emsdk/emscripten', | ||
| '-fdebug-compilation-dir=/emsdk/emscripten'] | ||
| cflags += [f'-ffile-prefix-map={relative_source_dir}={FAKE_EMSCRIPTEN_PATH}'] |
There was a problem hiding this comment.
Can you split this line out into its own PR? Something like "system_libs.py: Use the same deterministic path when building in batch mode". This seems like a separate and uncontroversial bugfix.
There was a problem hiding this comment.
Great!
Once #23222 lands maybe the next step should just be to unconditionally enable determinisic_paths? i.e. just remove the non-deterministic mode. We can always add it back later if it turns out someone wants it... but I doubt they wil.
There was a problem hiding this comment.
I thought what we were going to do was to unconditionally use -fmacro-prefix-map but use -ffile-prefix-map (which also enables -fdebug-prefix-map) only in deterministic_paths? This is basically what #23195 (comment) suggested but you also said maybe even that was not necessary... But anyway this PR does not remove the undeterministic mode. I can do either.
There was a problem hiding this comment.
I think just removing deterministic_paths and always doing it this way makes sense. Sorry for the back on forth on this.
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (4) test expectation files were updated by running the tests with `--rebaseline`: ``` other/codesize/test_codesize_cxx_except.size: 170928 => 170950 [+22 bytes / +0.01%] other/codesize/test_codesize_cxx_except_wasm.size: 142143 => 142154 [+11 bytes / +0.01%] other/codesize/test_codesize_cxx_except_wasm_exnref.size: 144730 => 144741 [+11 bytes / +0.01%] other/codesize/test_codesize_cxx_mangle.size: 232437 => 232468 [+31 bytes / +0.01%] Average change: +0.01% (+0.01% - +0.01%) ```
| cflags += [f'-ffile-prefix-map={source_dir}={DETERMINISITIC_PREFIX}', | ||
| f'-ffile-prefix-map={relative_source_dir}={DETERMINISITIC_PREFIX}', | ||
| f'-fdebug-compilation-dir={DETERMINISITIC_PREFIX}'] |
There was a problem hiding this comment.
if deterministic_paths part hasn't changed; I only merged two cflags += ... into one and changed the order of the flags.
When
deterministic_pathsis set, we are currently using-ffile-prefix-mapto produce a deterministic prefix (/emsdk/emscripten) in data and debug info, which was recently done in #23222 and #23225.-ffile-prefix-mapis an alias for both-fmacro-prefix-map, which affects__FILE__macro, and-fdebug-prefix-map, which affects the debug info section.This lets the contents of
__FILE__use the deterministic prefix/emsdk/emscriptenunconditionally, i.e., even ifdeterministic_pathsis not turned on. This is to produce reproducible builds and also make the code size tests' results consistent across platforms. The reason we still allow the real local path to be used in the debug info section whendeterministic_pathsis not set is to allow local developers to see their real paths.This is basically implementing what's suggested in #23195 (comment)
This also turns
deterministic_pathson for the Ninja path in embuilder for consistency with the non-Ninja path.Fixes #23195.