Explicitly set bulk memory and nontrapping-fptoint clang flags #22751
Conversation
|
Also /cc @walkingeyerobot since once it's finished this change will probably need to be mirrored for the bare-clang/non-emcc.py scenario. |
dschuff
changed the title
|
OK, assuming the tests are good, I think this should work to keep the feature set stable even as LLVM updates the defaults. Once we get the lowering passes in, and ensure our tests are correct, we can back this part out. |
I noticed these were a little out-of-place in emscripten-core#22751
I noticed these were a little out-of-place in #22751
| features = {} | ||
| feature_count = self.read_byte() | ||
| while self.tell() < section.offset + section.size: | ||
| prefix = TargetFeaturePrefix(self.read_byte()) |
There was a problem hiding this comment.
What does an argument to TargetFeaturePrefix do here?
There was a problem hiding this comment.
It just constructs the enum with that value.
| class TargetFeaturePrefix(IntEnum): | ||
| USED = 0x2b | ||
| DISALLOWED = 0x2d | ||
| REQUIRED = 0x3d |
There was a problem hiding this comment.
Do we still emit REQUIRED? I can't find anywhere in LLVM we emit it after llvm/llvm-project@3f34e1b. cc @tlively
There was a problem hiding this comment.
Not sure. Seems OK to include it in this enum though. For now we really only check for USED in the test.
There was a problem hiding this comment.
This whole system is slightly overengineered (my fault 🫣), and I'm not aware of any uses of REQUIRED. It would be a nice cleanup to remove it entirely here and in the tool conventions doc.
There was a problem hiding this comment.
| verify_features_sec_linked('nontrapping-fptoint', False) | ||
|
|
||
| compile(['-sMIN_SAFARI_VERSION=150000', '-mno-bulk-memory', '-sWASM_BIGINT']) | ||
| # FIXME? -mno-bulk-memory at link time does not override MIN_SAFARI_VERSION. it probably should? |
There was a problem hiding this comment.
one weirdness we have currently is that the -m flags (including -mno-signext currently) do not work at link time to disable features (and e.g. do not trigger the lowering passes). Only the the feature_matrix/browser version code does this. So there's currently no way to specify the feature behavior at link time on a granular level. I'm not sure what exactly the use case for this would be, but it wouldn't surprise me too much if there were one. Maybe we should think about that. (although it doesn't need to be this PR).
When clang starts enabling features by default that Emscripten's minimum browser versions want disabled, Emscripten will need to explicitly disable them. This PR does that; once clang rolls, we can update the default versions and tests in a single commit if needed.
See also llvm/llvm-project#112049