Ignore ASAN_SHADOW_SIZE #12249
Ignore ASAN_SHADOW_SIZE #12249
Conversation
Ignores the ASAN_SHADOW_SIZE setting (with a warning) and instead calculates the necessary shadow memory size from INITIAL_MEMORY, ALLOW_MEMORY_GROWTH, and MAXIMUM_MEMORY, increasing the initial and maximum memory by that amount. Closes emscripten-core#12135.
|
I have verified locally that this does not cause any additional asan tests to fail (and in fact causes one fewer test to fail) except for all the minimal runtime tests, as described in #12248, but that's a separate issue. |
| exit_with_error('ASan requires a finite MAXIMUM_MEMORY') | ||
|
|
||
| shadow_size = max_mem // 8 | ||
| shared.Settings.GLOBAL_BASE = shadow_size |
There was a problem hiding this comment.
What if the user sets GLOBAL_BASE? Perhaps we should not allow that in general?
emcc.py
Outdated
| max_mem = shared.Settings.INITIAL_MEMORY | ||
| if shared.Settings.ALLOW_MEMORY_GROWTH: | ||
| max_mem = shared.Settings.MAXIMUM_MEMORY | ||
| if max_mem < 0: |
There was a problem hiding this comment.
I think == -1 probably makes more sense here doesn't it?
emcc.py
Outdated
|
|
||
| shadow_size = max_mem // 8 | ||
| shared.Settings.GLOBAL_BASE = shadow_size | ||
| shared.Settings.INITIAL_MEMORY += shadow_size |
There was a problem hiding this comment.
Should this be shared.Settings.INITIAL_MEMORY += shadow_size - shared.Settings.GLOBAL_BASE (before you clobber GLOBAL_BASE that is).. normally the INITIAL_MEMORY includes the unused first 1024 bytes doesn't it?
There was a problem hiding this comment.
Oh, that makes sense. I wasn't really sure what GLOBAL_BASE was.
There was a problem hiding this comment.
Oh wait, I think that won't work because then INITIAL_MEMORY wouldn't be wasm-page aligned.
emcc.py
Outdated
| shared.Settings.GLOBAL_BASE = shadow_size | ||
| shared.Settings.INITIAL_MEMORY += shadow_size | ||
| if shared.Settings.ALLOW_MEMORY_GROWTH: | ||
| shared.Settings.MAXIMUM_MEMORY += shadow_size |
There was a problem hiding this comment.
Are these changes needed? It seems like normally the user-available memory is [1024, max] After you raise GLOBAL_BASE, it would be [~max/8, max]. The user has lost around 1/8 of available memory, but that seems ok? They can always raise MAXIMUM_MEMORY. Similar things can happen when switching between say dlmalloc and emmalloc (as dlmalloc has more efficient metadata usage). So it could be simpler to just bump GLOBAL_BASE and that's it unless I'm missing something.
(I guess some of the shadow memory would not be used - since available memory is slightly less than a multiple of 8 - but that seems fine too?)
There was a problem hiding this comment.
I wanted to spare the user from having to use different memory settings in their ASan builds and also avoid the situation where the user gets a hard error because the calculated shadow size is greater than their initial size. So I would say these changes aren't necessary, but they were deliberately made with usability in mind.
There was a problem hiding this comment.
The user can always increase memory size in both normal and ASan builds (so that it is enough for ASan, and slightly more than needed normally).
But how can that hard error happen? I'm missing something.
There was a problem hiding this comment.
That would happen if initial_memory < maximum_memory / 8. Shadow memory has to be statically allocated up front and can't grow.
Wouldn't it be better to keep the production build memory lower, though?
There was a problem hiding this comment.
Oh, I see. Yeah, that's a potential annoyance, I agree. But at least it would be a clear error.
Yes, production builds matter more. I guess it seems unlikely to me for someone to want to force the same amount in both. There is no precise guarantee of how much memory is available with malloc anyhow due to metadata, so it's all ballpark-esque.
Another reason I'd prefer to not change these values is that it can be surprising if the user asks for X MB and then runs the ASan build and sees it is using more. It seems simpler and less confusing to not change values silently.
But I don't feel very strongly, if I'm the only one that prefers to not change them then let's do the other way.
There was a problem hiding this comment.
(Anyhow in the long term multimemory is the real solution here.)
There was a problem hiding this comment.
Yeah I totally see the arguments for doing it a different way as well. @sbc100, do you have an opinion on which way is more user-friendly?
|
I think alon makes a good point... asking users who need to to increase max memory for asan builds seems reasonable. I mean the mem requirements of native programs would also increase when asan was added so if you allocated a max memory to your native program you would need to increase it in the asan case too. |
|
Ok, sounds reasonable. I'll change this to carve the shadow memory out of the user-provided limits. |
| asan = make_run('asan', emcc_args=['-fsanitize=address', '-O2'], settings={'ALLOW_MEMORY_GROWTH': 1, 'INITIAL_MEMORY': 314572800}) | ||
| asani = make_run('asani', emcc_args=['-fsanitize=address', '-O2', '--pre-js', os.path.join(os.path.dirname(__file__), 'asan-no-leak.js')], | ||
| settings={'ALLOW_MEMORY_GROWTH': 1, 'MAXIMUM_MEMORY': 1879048192}) | ||
| settings={'ALLOW_MEMORY_GROWTH': 1, 'INITIAL_MEMORY': 314572800}) |
There was a problem hiding this comment.
does this mean that without setting MAXIMUM_MEMORY we use the default 2GB, which means shadow memory is 256MB?
if so that may not be best for CI, as each test will use that much memory immediately. maybe worth keeping the maximum?
There was a problem hiding this comment.
Yes, this goes back to using the default MAXIMUM_MEMORY, but we were already doing that before this PR. The MAXIMUM_MEMORY that we used to have in this PR was just 7/8 of the default, so not a whole lot better.
I could believe that this might cause CI issues, but it hasn't so far, so I think that would be better addressed in a separate PR.
There was a problem hiding this comment.
Oh, I was reading the diff wrong. sgtm.
Last question: why add -O2 here?
There was a problem hiding this comment.
The SIMD tests were failing because -O0 combined with ASan was leading to functions with too many locals. Those failures aren't related to this change, so I could remove the -O2 from this PR, but adding it didn't seem to have any downsides, either.
3 participants
Ignores the ASAN_SHADOW_SIZE setting (with a warning) and instead calculates the
necessary shadow memory size from INITIAL_MEMORY, ALLOW_MEMORY_GROWTH, and
MAXIMUM_MEMORY, increasing the initial and maximum memory by that amount.
Closes #12135.