Fix mis-aligned access issues with s390x #4702
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
cranelift
bjorn3
reviewed
Aug 12, 2022
| @@ -342,22 +342,18 @@ impl Module for ObjectModule { | |||
| } | |||
| *defined = true; | |||
|
|
|||
| let align = std::cmp::max(self.function_alignment, self.isa.symbol_alignment()); | |||
There was a problem hiding this comment.
I think you should default function_alignment to isa.symbol_alignment().
There was a problem hiding this comment.
I didn't do that because it seemed that would allow a caller to override the function alignment to smaller than isa.symbol_alignment(), and that simply doesn't work on s390x.
cfallin
reviewed
Aug 15, 2022
| @@ -21,6 +21,7 @@ pub fn mem_finalize( | |||
| have_d12: bool, | |||
| have_d20: bool, | |||
| have_pcrel: bool, | |||
| have_unaligned_pcrel: bool, | |||
There was a problem hiding this comment.
This list of flags is long enough now that it's starting to feel a bit error-prone; and e.g. the diff adding false in the right places is hard to review carefully. Could we turn this into a struct of flags?
There was a problem hiding this comment.
Agreed, that makes sense. Like this?
This fixes two problems: minimum symbol alignment for the LARL instruction, and alignment requirements for LRL/LGRL etc. The first problem is that the LARL instruction used to load a symbol address (PC relative) requires that the target symbol is at least 2-byte aligned. This is always guaranteed for code symbols (all instructions must be 2-aligned anyway), but not necessarily for data symbols. Other s390x compilers fix this problem by ensuring that all global symbols are always emitted with a minimum 2-byte alignment. This patch introduces an equivalent mechanism for cranelift: - Add a symbol_alignment routine to TargetIsa, similar to the existing code_section_alignment routine. - Respect symbol_alignment as minimum alignment for all symbols emitted in the object backend (code and data). The second problem is that PC-relative instructions that directly *access* data (like LRL/LGRL, STRL/STGRL etc.) not only have the 2-byte requirement like LARL, but actually require that their memory operand is *naturally* aligned (i.e. alignment is at least the size of the access). This property (natural alignment for memory accesses) is supposed to be provided by the "aligned" flag in MemFlags; however, this is not implemented correctly at the moment. To fix this, this patch: - Only emits PC-relative memory access instructions if the "aligned" flag is set in the associated MemFlags. - Fixes a bug in emit_small_memory_copy and emit_small_memset which currently set the aligned flag unconditionally, ignoring the actual alignment info passed by their caller. Tested with wasmtime and cg_clif.
uweigand
force-pushed
the
s390x-align-fixes
branch
from
3d949d7
to
41c42f4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes two problems: minimum symbol alignment for the LARL
instruction, and alignment requirements for LRL/LGRL etc.
The first problem is that the LARL instruction used to load a
symbol address (PC relative) requires that the target symbol
is at least 2-byte aligned. This is always guaranteed for code
symbols (all instructions must be 2-aligned anyway), but not
necessarily for data symbols.
Other s390x compilers fix this problem by ensuring that all
global symbols are always emitted with a minimum 2-byte
alignment. This patch introduces an equivalent mechanism
for cranelift:
existing code_section_alignment routine.
emitted in the object backend (code and data).
The second problem is that PC-relative instructions that
directly access data (like LRL/LGRL, STRL/STGRL etc.)
not only have the 2-byte requirement like LARL, but actually
require that their memory operand is naturally aligned
(i.e. alignment is at least the size of the access).
This property (natural alignment for memory accesses) is
supposed to be provided by the "aligned" flag in MemFlags;
however, this is not implemented correctly at the moment.
To fix this, this patch:
"aligned" flag is set in the associated MemFlags.
which currently set the aligned flag unconditionally, ignoring
the actual alignment info passed by their caller.
Tested with wasmtime and cg_clif.
FYI @cfallin @bjorn3 - this is the last missing cranelift patch to make cg_clif work on s390x.