Skip to content

Fix Asan ( address sanitizer ) related errors - #3

Merged
thefirehacker merged 33 commits into
AIEdX:mainfrom
emscripten-core:main
Jan 5, 2022
Merged

Fix Asan ( address sanitizer ) related errors#3
thefirehacker merged 33 commits into
AIEdX:mainfrom
emscripten-core:main

Conversation

@thefirehacker

Copy link
Copy Markdown
Member

No description provided.

sbc100 and others added 30 commits December 16, 2021 08:22
This should avoid us ever forgetting to add opt flags to a system library
as was discovered in #15723. It also is a nice decrease in python code.

After that PR, all libraries have a specified opt level, and so this PR is
NFC: it just sets the default and removes code that specifies the same
value as the default.

As an extra check that this has no noticeable effect, I verified that code
size and metadce rebaselining with this patch leads to identical results
as without the patch. I also checked that the order of cflags is correct,
that is, when a library says -Oz that appears after the default -O2. And
a few tests break if that order were wrong (like test_metadce_cxx_except)
so we'd notice any mistakes here.
- Changing the current working directory and then assigning the target dir to an ancestor should not be allowed.
- Edited `test_rename` to reflect this.
…15515)

- This return prevents the result from actually being handled.
- The argument list didn't match the C header.
This bug was caused by miscalculating the adjustments made to TOTAL_MEMORY
and GLOBAL_BASE and using ASan.  The bug was introduced in #14631 and
causes ASan to write poison values over static data in some cases.

This bug was really only effecting builds without ALLOW_MEMORY_GROWTH
since when ALLOW_MEMORY_GROWTH is enabled that shadow region is very
large and unless users were use a lot of memory it was unlikely ASan
would write to the end of it (and clobber static data).

As well as fixing the calculations, this change also adds a runtime
assert to ensure these regions never overlap (at least not at startup).
This should catch regressions in these calculations.

In order to test this case I removed ALLOW_MEMORY_GROWTH form several
asan and lsan tests.  It should not longer be necessary to set
`ALLOW_MEMORY_GROWTH` or adjust `TOTAL_MEMORY` when using sanitizers
these days.  The `asan` suite in the core tests still uses
`ALLOW_MEMORY_GROWTH`.  As a followup we might want to look into
removing that too.

Fixes: #15762
It seems this was mistakenly checked in as part of #14375.
With this change we allow these features by default.  If a user
explicitly opts into older browser support we trigger the running
of closure compiler with `--language_out ES5 --compilation_level
WHITESPACE_ONLY`.

For most users this will be a code size win, but otherwise a no-op.

For users who need older browser support they will now have their
output run though closure by default.  If they want to take a care
of the transpilation process themselves rather than have emscripten
to it auto-magically they can run with `--closure=0`.

When we auto-magically run closure to do transpilation we generate
a warning.  This warning can be suppressed by add `--closure=1` to
explicitly opt in, or `--closure=0` to explicitly opt out.

This change in does not actually include any usage of these features
and so don't include the code size benefits. Those will be part of
followup PRs.

Fixes: #11984
When running Emscripten-generated scripts via `node -e "<script>"`,
the `fs` variable is already defined and so `nodePath` was never being
initialized.
This was removed from the WebGPU spec, and has also now been removed
from webgpu-native/webgpu-headers and from Dawn.
Now that ES6 features are permitted in emscripten JS library
code (see #15763) we can take advantage of some of them to
reduce size of our output JS.

This is just an initial test of using ES6 features.

See: #11984
Followup to #15575 which removed that last uses of these objects.
Linking multiple system libraries could produce a different result,
because sets in Python doesn't preserve insertion order.

This change fixes this non-determinism behavior by removing the set
in favor of a list, which is guaranteed to keep the elements in order
in which they were inserted.

See: #15706.
The use of this field was removed in #12643.
This information is part of the dylink section so there is no
need for this metadata field anymore.
This change does not effect the JS that we output, only the the internal
JS tooling.

This is a followup to #15836 which did this cleanup just for a single
file.

The long list of exclusions in `.eslintrc.yml` is because these files
are part of the JS library code which we ship and we a lot of that
cannot be run though the lint tool because it uses our speciall
pre-processing techniques.
This 6 byte in create was due to #15805.  This suggests we should
probably be measuring codesize with `--closure=1`.
This makes LTO work with Wasm SjLj.

For exceptions, we have `-fwasm-exceptions` option, which was created to
match other kinds of exceptions (DWARF, SjLj, and SEH). Clang driver
checks if `-fwasm-exceptions` is given, and if so does these things:
- Adds `target-features=+exception-handling` attribute to LLVM IR
- Adds `-mllvm -wasm-enable-eh` to the backend options.

For SjLj, we couldn't create such option like `-fwasm-exceptions`,
because other platforms all enable it by default and doesn't have a
dedicated Clang option for that. So we are passing
`-mllvm -wasm-enable-sjlj` in emcc to Clang as an workaround. Clang
driver checks if `-mllvm -wasm-enable-sjlj` is given, and if so adds
`target-features=+exception-handling` attribute to LLVM IR too. (Unlike
EH, it doesn't need to add `-mllvm -wasm-enable-sjlj` again because we
are already using that option)

The problem is, `-mllvm` options are not Clang options but LLVM options,
and we process them in `llvm_backend_args`. In non-LTO mode, these
options are added to Clang, but in LTO mode, these are added to wasm-ld.
But as mentioned above, we are using `-mllvm -wasm-enable-eh` as a kind
of Clang option due to lack of a dedicated Clang option like
`-fwasm-exceptions`. This adds `target-features=+exception-handling`
attribute to LLVM IR, which should be done in Clang and cannot be done
in wasm-ld.

We cannot just add `-mllvm -wasm-enable-eh` to `get_cflags`, because
then it will not be added to wasm-ld. The options is necessary in the
backend processing as well. We cannot add it to both `get_cflags` and
`llvm_backend_args` either because it will error out saying the same
option is given twice in non-LTO mode.

So this PR ads `-mexception-handling`, which enables EH feature, to
Clang, in case we use Wasm SjLj. (Wasm EH doesn't need this because
`-fwasm-exceptions` enables it in Clang.) This works for both LTO and
non-LTO modes.

Fixes #15665.
When an error occurs on a pthread, attempt to report the thread ID
Adds notes discussed in #14354 and #15592 , specifically
about AUTHORS being optional, and linking to the code style docs.
domoritz and others added 3 commits January 4, 2022 11:46
* Update SDL2 port to point at upstream: we no longer need a fork in
  emscripten-ports.

* Avoid calling SDL_Quit in SDL2 tests

This destroys the window on newer SDL, causing the tests to fail as the
rendered result is gone.

* Fix sdl_togglefullscreen for new event target behaviour

* Remove creating a renderer from test_webgl_context_attributes_sdl2

Seems some state from the renderer is causing the context created later to not draw anything. This test doesn't use the renderer API anyway, so remove it.

* Update changelog
@thefirehacker

Copy link
Copy Markdown
Member Author

Most fix are related to a bug fix "Adjust initial memory to handle asan shadow size"

Full thread of issues and PR emscripten-core#14631

@thefirehacker
thefirehacker merged commit 695d481 into AIEdX:main Jan 5, 2022
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.