[mlir][ByteCodeReader] Fix bugs in EncodingReader::alignTo #140660
Conversation
It seems that the number of padding value should be computed based on the offset of the current pointer to the bytecode buffer begin (instead of the absolute address of `ptr`). Otherwise, the skipped padding value will be dependent on the alignment of `buffer.begin()`?
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Peiming Liu (PeimingLiu) ChangesIt seems that the number of padding value should be computed based on the relative distance between Or is there any implicit assumption on the alignment of buffer that holds the bytecode? Full diff: https://github.com/llvm/llvm-project/pull/140660.diff 1 Files Affected:
diff --git a/mlir/lib/Bytecode/Reader/BytecodeReader.cpp b/mlir/lib/Bytecode/Reader/BytecodeReader.cpp
index 1052946d4550b..793170dcc594c 100644
--- a/mlir/lib/Bytecode/Reader/BytecodeReader.cpp
+++ b/mlir/lib/Bytecode/Reader/BytecodeReader.cpp
@@ -107,7 +107,8 @@ class EncodingReader {
return emitError("expected alignment to be a power-of-two");
auto isUnaligned = [&](const uint8_t *ptr) {
- return ((uintptr_t)ptr & (alignment - 1)) != 0;
+ unsigned offset = ptr - buffer.begin();
+ return (offset & (alignment - 1)) != 0;
};
// Shift the reader position to the next alignment boundary.
@@ -1506,7 +1507,7 @@ class mlir::BytecodeReader::Impl {
UseListOrderStorage(bool isIndexPairEncoding,
SmallVector<unsigned, 4> &&indices)
: indices(std::move(indices)),
- isIndexPairEncoding(isIndexPairEncoding){};
+ isIndexPairEncoding(isIndexPairEncoding) {};
/// The vector containing the information required to reorder the
/// use-list of a value.
SmallVector<unsigned, 4> indices;
|
|
The alignment in the reader must be for the actual pointer address, because it gets used to directly load/read data that has alignment requirements. There is an expectation that whoever created the buffer (by alloc or mmap) ensures that the alignment is at least to the amount expected, though in a majority of cases (unless you have buffers with large alignment constraints) it already is given malloc gives you at least max_align_t |
River707
left a comment
River707
left a comment
There was a problem hiding this comment.
Marking RC because this is not the expected reader behavior.
|
Make sense, that was why I had the question "Or is there any implicit assumption on the alignment of buffer that holds the bytecode?"
What is the expected amount ( I might have missed important pieces, but it still seems somehow unsafe. E.g., It is only guaranteed to be safe when the buffer is aligned to the maximum section alignments IIUC. |
It seems that the number of padding value should be computed based on the relative distance between
ptrandbuffer.begin()(instead of the absolute address ofptr). Otherwise, the skipped padding value will be indeterministic and depends on the initial alignment ofbuffer.begin()?Or is there any implicit assumption on the alignment of buffer that holds the bytecode?
See
llvm-project/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
Line 198 in 755acb1