Skip to content

Commit ce3aff1

Browse files
committed
[Basic] Always serialize integers in little-endian byte order
This change fixes the ExponentialGrowthAppendingBinaryByteStream tests on big endian machines. Force ExponentialGrowthAppendingBinaryByteStreams to use little- endian byte order. We always used little-endian byte order anyway and it seems very unlikely we'll need the flexibility to make the stream big-endian in the future. The benefit of this is that we can use portable APIs while still allowing the compiler to remove conditional byte swaps. Also replace writeRaw with writeInteger and make it explicitly little-endian to make the API cleaner and more portable.
1 parent af0291b commit ce3aff1

File tree

5 files changed

+30
-43
lines changed

5 files changed

+30
-43
lines changed

Diff for: include/swift/Basic/ByteTreeSerialization.h

+9-12
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ class ByteTreeWriter {
155155
llvm::BinaryStreamWriter &StreamWriter;
156156

157157
/// The underlying stream of the StreamWriter. We need this reference so that
158-
/// we can call \c ExponentialGrowthAppendingBinaryByteStream.writeRaw
158+
/// we can call \c ExponentialGrowthAppendingBinaryByteStream.writeInteger
159159
/// which is more efficient than the generic \c writeBytes of
160160
/// \c llvm::BinaryStreamWriter since it avoids the arbitrary size memcopy.
161161
ExponentialGrowthAppendingBinaryByteStream &Stream;
@@ -179,13 +179,10 @@ class ByteTreeWriter {
179179
llvm::BinaryStreamWriter &StreamWriter, UserInfoMap &UserInfo)
180180
: StreamWriter(StreamWriter), Stream(Stream), UserInfo(UserInfo) {}
181181

182-
/// Write the given value to the ByteTree in the same form in which it is
183-
/// represented on the serializing machine.
182+
/// Write the given value to the ByteTree in little-endian byte order.
184183
template <typename T>
185-
llvm::Error writeRaw(T Value) {
186-
// FIXME: We implicitly inherit the endianess of the serializing machine.
187-
// Since we're currently only supporting macOS that's not a problem for now.
188-
auto Error = Stream.writeRaw(StreamWriter.getOffset(), Value);
184+
llvm::Error writeInteger(T Value) {
185+
auto Error = Stream.writeInteger(StreamWriter.getOffset(), Value);
189186
StreamWriter.setOffset(StreamWriter.getOffset() + sizeof(T));
190187
return Error;
191188
}
@@ -205,7 +202,7 @@ class ByteTreeWriter {
205202
// Set the most significant bit to indicate that the next construct is an
206203
// object and not a scalar.
207204
uint32_t ToWrite = NumFields | (1 << 31);
208-
auto Error = writeRaw(ToWrite);
205+
auto Error = writeInteger(ToWrite);
209206
(void)Error;
210207
assert(!Error);
211208

@@ -241,7 +238,7 @@ class ByteTreeWriter {
241238
llvm::BinaryStreamWriter StreamWriter(Stream);
242239
ByteTreeWriter Writer(Stream, StreamWriter, UserInfo);
243240

244-
auto Error = Writer.writeRaw(ProtocolVersion);
241+
auto Error = Writer.writeInteger(ProtocolVersion);
245242
(void)Error;
246243
assert(!Error);
247244

@@ -272,7 +269,7 @@ class ByteTreeWriter {
272269
// bitflag that indicates if the next construct in the tree is an object
273270
// or a scalar.
274271
assert((ValueSize & ((uint32_t)1 << 31)) == 0 && "Value size too large");
275-
auto SizeError = writeRaw(ValueSize);
272+
auto SizeError = writeInteger(ValueSize);
276273
(void)SizeError;
277274
assert(!SizeError);
278275

@@ -292,11 +289,11 @@ class ByteTreeWriter {
292289
validateAndIncreaseFieldIndex(Index);
293290

294291
uint32_t ValueSize = sizeof(T);
295-
auto SizeError = writeRaw(ValueSize);
292+
auto SizeError = writeInteger(ValueSize);
296293
(void)SizeError;
297294
assert(!SizeError);
298295

299-
auto ContentError = writeRaw(Value);
296+
auto ContentError = writeInteger(Value);
300297
(void)ContentError;
301298
assert(!ContentError);
302299
}

Diff for: include/swift/Basic/ExponentialGrowthAppendingBinaryByteStream.h

+9-16
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,10 @@ class ExponentialGrowthAppendingBinaryByteStream
3333
/// The buffer holding the data.
3434
SmallVector<uint8_t, 0> Data;
3535

36-
llvm::support::endianness Endian;
36+
/// Data in the stream is always encoded in little-endian byte order.
37+
const llvm::support::endianness Endian = llvm::support::endianness::little;
3738
public:
38-
ExponentialGrowthAppendingBinaryByteStream()
39-
: ExponentialGrowthAppendingBinaryByteStream(
40-
llvm::support::endianness::little) {}
41-
ExponentialGrowthAppendingBinaryByteStream(llvm::support::endianness Endian)
42-
: Endian(Endian) {}
39+
ExponentialGrowthAppendingBinaryByteStream() = default;
4340

4441
void reserve(size_t Size);
4542

@@ -57,16 +54,11 @@ class ExponentialGrowthAppendingBinaryByteStream
5754

5855
llvm::Error writeBytes(uint32_t Offset, ArrayRef<uint8_t> Buffer) override;
5956

60-
/// This is an optimized version of \c writeBytes that assumes we know the
61-
/// size of \p Value at compile time (which in particular holds for integers).
62-
/// It does so by exposing the memcpy to the optimizer along with the size
63-
/// of the value being assigned; the compiler can then optimize the memcpy
64-
/// into a fixed set of instructions.
65-
/// This assumes that the endianess of this steam is the same as the native
66-
/// endianess on the executing machine. No endianess transformations are
67-
/// performed.
57+
/// This is an optimized version of \c writeBytes specifically for integers.
58+
/// Integers are written in little-endian byte order.
6859
template<typename T>
69-
llvm::Error writeRaw(uint32_t Offset, T Value) {
60+
llvm::Error writeInteger(uint32_t Offset, T Value) {
61+
static_assert(std::is_integral<T>::value, "Integer required.");
7062
if (auto Error = checkOffsetForWrite(Offset, sizeof(T))) {
7163
return Error;
7264
}
@@ -77,7 +69,8 @@ class ExponentialGrowthAppendingBinaryByteStream
7769
Data.resize(RequiredSize);
7870
}
7971

80-
::memcpy(Data.data() + Offset, &Value, sizeof Value);
72+
llvm::support::endian::write<T, llvm::support::unaligned>(
73+
Data.data() + Offset, Value, Endian);
8174

8275
return llvm::Error::success();
8376
}

Diff for: tools/SourceKit/tools/sourcekitd/lib/API/Requests.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -2651,8 +2651,7 @@ void serializeSyntaxTreeAsByteTree(
26512651
ResponseBuilder::Dictionary &Dict) {
26522652
auto StartClock = clock();
26532653
// Serialize the syntax tree as a ByteTree
2654-
swift::ExponentialGrowthAppendingBinaryByteStream Stream(
2655-
llvm::support::endianness::little);
2654+
auto Stream = swift::ExponentialGrowthAppendingBinaryByteStream();
26562655
Stream.reserve(32 * 1024);
26572656
std::map<void *, void *> UserInfo;
26582657
UserInfo[swift::byteTree::UserInfoKeyReusedNodeIds] = &ReusedNodeIds;

Diff for: tools/swift-syntax-test/swift-syntax-test.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -735,8 +735,7 @@ int doSerializeRawTree(const char *MainExecutablePath,
735735
return EXIT_FAILURE;
736736
}
737737

738-
swift::ExponentialGrowthAppendingBinaryByteStream Stream(
739-
llvm::support::endianness::little);
738+
auto Stream = ExponentialGrowthAppendingBinaryByteStream();
740739
Stream.reserve(32 * 1024);
741740
std::map<void *, void *> UserInfo;
742741
UserInfo[swift::byteTree::UserInfoKeyReusedNodeIds] = &ReusedNodeIds;

Diff for: unittests/Basic/ExponentialGrowthAppendingBinaryByteStreamTests.cpp

+10-11
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
#include "gtest/gtest.h"
1919

2020
using namespace llvm;
21-
using namespace llvm::support;
2221
using namespace swift;
2322

2423
class ExponentialGrowthAppendingBinaryByteStreamTest : public testing::Test {};
@@ -27,7 +26,7 @@ class ExponentialGrowthAppendingBinaryByteStreamTest : public testing::Test {};
2726
// unittests/BinaryStreamTests.cpp in the LLVM project
2827
TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, ReadAndWrite) {
2928
StringRef Strings[] = {"1", "2", "3", "4"};
30-
ExponentialGrowthAppendingBinaryByteStream Stream(support::little);
29+
auto Stream = ExponentialGrowthAppendingBinaryByteStream();
3130

3231
BinaryStreamWriter Writer(Stream);
3332
BinaryStreamReader Reader(Stream);
@@ -76,7 +75,7 @@ TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, ReadAndWrite) {
7675
}
7776

7877
TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, WriteAtInvalidOffset) {
79-
ExponentialGrowthAppendingBinaryByteStream Stream(llvm::support::little);
78+
auto Stream = ExponentialGrowthAppendingBinaryByteStream();
8079
EXPECT_EQ(0U, Stream.getLength());
8180

8281
std::vector<uint8_t> InputData = {'T', 'e', 's', 't', 'T', 'e', 's', 't'};
@@ -97,7 +96,7 @@ TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, WriteAtInvalidOffset) {
9796
TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, InitialSizeZero) {
9897
// Test that the stream also works with an initial size of 0, which doesn't
9998
// grow when doubled.
100-
ExponentialGrowthAppendingBinaryByteStream Stream(llvm::support::little);
99+
auto Stream = ExponentialGrowthAppendingBinaryByteStream();
101100

102101
std::vector<uint8_t> InputData = {'T', 'e', 's', 't'};
103102
auto Test = makeArrayRef(InputData).take_front(4);
@@ -106,7 +105,7 @@ TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, InitialSizeZero) {
106105
}
107106

108107
TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, GrowMultipleSteps) {
109-
ExponentialGrowthAppendingBinaryByteStream Stream(llvm::support::little);
108+
auto Stream = ExponentialGrowthAppendingBinaryByteStream();
110109

111110
// Test that the buffer can grow multiple steps at once, e.g. 1 -> 2 -> 4
112111
std::vector<uint8_t> InputData = {'T', 'e', 's', 't'};
@@ -116,7 +115,7 @@ TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, GrowMultipleSteps) {
116115
}
117116

118117
TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, WriteIntoMiddle) {
119-
ExponentialGrowthAppendingBinaryByteStream Stream(llvm::support::little);
118+
auto Stream = ExponentialGrowthAppendingBinaryByteStream();
120119

121120
// Test that the stream resizes correctly if we write into its middle
122121
std::vector<uint8_t> InitialData = {'T', 'e', 's', 't'};
@@ -143,16 +142,16 @@ TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, WriteIntoMiddle) {
143142
EXPECT_EQ(6u, Stream.getLength());
144143
}
145144

146-
TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, WriteRaw) {
147-
ExponentialGrowthAppendingBinaryByteStream Stream(llvm::support::little);
145+
TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, WriteInteger) {
146+
auto Stream = ExponentialGrowthAppendingBinaryByteStream();
148147

149-
// Test the writeRaw method
148+
// Test the writeInteger method
150149
std::vector<uint8_t> InitialData = {'H', 'e', 'l', 'l', 'o'};
151150
auto InitialDataRef = makeArrayRef(InitialData);
152151
EXPECT_THAT_ERROR(Stream.writeBytes(0, InitialDataRef), Succeeded());
153152
EXPECT_EQ(InitialDataRef, Stream.data());
154153

155-
EXPECT_THAT_ERROR(Stream.writeRaw(5, (uint8_t)' '), Succeeded());
154+
EXPECT_THAT_ERROR(Stream.writeInteger(5, (uint8_t)' '), Succeeded());
156155
std::vector<uint8_t> AfterFirstInsert = {'H', 'e', 'l', 'l', 'o', ' '};
157156
auto AfterFirstInsertRef = makeArrayRef(AfterFirstInsert);
158157
EXPECT_EQ(AfterFirstInsertRef, Stream.data());
@@ -162,7 +161,7 @@ TEST_F(ExponentialGrowthAppendingBinaryByteStreamTest, WriteRaw) {
162161
'o' << 8 |
163162
'r' << 16 |
164163
'l' << 24;
165-
EXPECT_THAT_ERROR(Stream.writeRaw(6, ToInsert), Succeeded());
164+
EXPECT_THAT_ERROR(Stream.writeInteger(6, ToInsert), Succeeded());
166165
std::vector<uint8_t> AfterSecondInsert = {'H', 'e', 'l', 'l', 'o', ' ',
167166
'w', 'o', 'r', 'l'};
168167
auto AfterSecondInsertRef = makeArrayRef(AfterSecondInsert);

0 commit comments

Comments
 (0)