Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for signed/bool to BufferReader/Writer #27637

Merged
merged 2 commits into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/lib/core/CHIPEncoding.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,13 @@ namespace LittleEndian {
template <typename T>
inline T HostSwap(T v);

// For completeness of the set, we have identity.
template <>
inline uint8_t HostSwap<uint8_t>(uint8_t v)
{
return v;
}

/**
* This conditionally performs, as necessary for the target system, a
* byte order swap by value of the specified 16-bit value, presumed to
Expand Down
51 changes: 32 additions & 19 deletions src/lib/support/BufferReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,40 +17,46 @@

#include "BufferReader.h"

#include <lib/core/CHIPEncoding.h>

#include <string.h>
#include <type_traits>

namespace chip {
namespace Encoding {
namespace LittleEndian {

namespace {
// These helper methods return void and put the value being read into an

// This helper methods return void and put the value being read into an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"method returns" and "puts"

// outparam because that allows us to easily overload on the type of the
// thing being read.
void ReadHelper(const uint8_t *& p, uint8_t * dest)
{
*dest = Read8(p);
}
void ReadHelper(const uint8_t *& p, uint16_t * dest)
{
*dest = Read16(p);
}
void ReadHelper(const uint8_t *& p, uint32_t * dest)
void ReadHelper(const uint8_t * p, bool * dest)
{
*dest = Read32(p);
*dest = (*p != 0);
}
void ReadHelper(const uint8_t *& p, uint64_t * dest)

template <typename T>
void ReadHelper(const uint8_t * p, T * dest)
{
*dest = Read64(p);
std::make_unsigned_t<T> result;
memcpy(&result, p, sizeof(result));
result = chip::Encoding::LittleEndian::HostSwap(result);

*dest = static_cast<T>(result);
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple Jul 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting to signed of things that are "negative" (i.e. out of range) has implementation-defined behavior until c++20...

This should probably use CastToSigned for the casting; that's what we have that utility for.

}

} // anonymous namespace

template <typename T>
void Reader::RawRead(T * retval)
void Reader::RawReadLowLevelBeCareful(T * retval)
{
static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing");
static_assert((-1 & 3) == 3, "LittleEndian::BufferReader only works with 2's complement architectures.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the part that's meant to make the static_cast above safe? Or is this something we depend on for the HostSwap bits to work right for negative numbers? Would be good to make it clearer what the actual 2's complement dependency is here.


VerifyOrReturn(IsSuccess());

static constexpr size_t data_size = sizeof(T);
constexpr size_t data_size = sizeof(T);

if (mAvailable < data_size)
{
Expand All @@ -61,6 +67,8 @@ void Reader::RawRead(T * retval)
}

ReadHelper(mReadPtr, retval);
mReadPtr += data_size;

mAvailable = static_cast<uint16_t>(mAvailable - data_size);
}

Expand All @@ -84,10 +92,15 @@ Reader & Reader::ReadBytes(uint8_t * dest, size_t size)
}

// Explicit Read instantiations for the data types we want to support.
template void Reader::RawRead(uint8_t *);
template void Reader::RawRead(uint16_t *);
template void Reader::RawRead(uint32_t *);
template void Reader::RawRead(uint64_t *);
template void Reader::RawReadLowLevelBeCareful(bool *);
template void Reader::RawReadLowLevelBeCareful(int8_t *);
template void Reader::RawReadLowLevelBeCareful(int16_t *);
template void Reader::RawReadLowLevelBeCareful(int32_t *);
template void Reader::RawReadLowLevelBeCareful(int64_t *);
template void Reader::RawReadLowLevelBeCareful(uint8_t *);
template void Reader::RawReadLowLevelBeCareful(uint16_t *);
template void Reader::RawReadLowLevelBeCareful(uint32_t *);
template void Reader::RawReadLowLevelBeCareful(uint64_t *);

} // namespace LittleEndian
} // namespace Encoding
Expand Down
100 changes: 95 additions & 5 deletions src/lib/support/BufferReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,28 @@ class Reader
CHECK_RETURN_VALUE
CHIP_ERROR StatusCode() const { return mStatus; }

/**
* @return false if the reader is in error, true if the reader is OK.
*/
bool IsSuccess() const { return StatusCode() == CHIP_NO_ERROR; }

/**
* Read a bool, assuming single byte storage.
*
* @param [out] dest Where the 8-bit integer goes.
*
* @note The read can put the reader in a failed-status state if there are
* not enough octets available. Callers must either continue to do
* more reads on the return value or check its status to see whether
* the sequence of reads that has been performed succeeded.
*/
CHECK_RETURN_VALUE
Reader & ReadBool(bool * dest)
{
RawReadLowLevelBeCareful(dest);
return *this;
}

/**
* Read a single 8-bit unsigned integer.
*
Expand All @@ -103,7 +125,7 @@ class Reader
CHECK_RETURN_VALUE
Reader & Read8(uint8_t * dest)
{
RawRead(dest);
RawReadLowLevelBeCareful(dest);
return *this;
}

Expand All @@ -120,7 +142,7 @@ class Reader
CHECK_RETURN_VALUE
Reader & Read16(uint16_t * dest)
{
RawRead(dest);
RawReadLowLevelBeCareful(dest);
return *this;
}

Expand All @@ -137,7 +159,7 @@ class Reader
CHECK_RETURN_VALUE
Reader & Read32(uint32_t * dest)
{
RawRead(dest);
RawReadLowLevelBeCareful(dest);
return *this;
}

Expand All @@ -154,7 +176,75 @@ class Reader
CHECK_RETURN_VALUE
Reader & Read64(uint64_t * dest)
{
RawRead(dest);
RawReadLowLevelBeCareful(dest);
return *this;
}

/**
* Read a single 8-bit signed integer.
*
* @param [out] dest Where the 8-bit integer goes.
*
* @note The read can put the reader in a failed-status state if there are
* not enough octets available. Callers must either continue to do
* more reads on the return value or check its status to see whether
* the sequence of reads that has been performed succeeded.
*/
CHECK_RETURN_VALUE
Reader & ReadSigned8(int8_t * dest)
{
RawReadLowLevelBeCareful(dest);
return *this;
}

/**
* Read a single 16-bit signed integer.
*
* @param [out] dest Where the 16-bit integer goes.
*
* @note The read can put the reader in a failed-status state if there are
* not enough octets available. Callers must either continue to do
* more reads on the return value or check its status to see whether
* the sequence of reads that has been performed succeeded.
*/
CHECK_RETURN_VALUE
Reader & ReadSigned16(int16_t * dest)
{
RawReadLowLevelBeCareful(dest);
return *this;
}

/**
* Read a single 32-bit signed integer.
*
* @param [out] dest Where the 32-bit integer goes.
*
* @note The read can put the reader in a failed-status state if there are
* not enough octets available. Callers must either continue to do
* more reads on the return value or check its status to see whether
* the sequence of reads that has been performed succeeded.
*/
CHECK_RETURN_VALUE
Reader & ReadSigned32(int32_t * dest)
{
RawReadLowLevelBeCareful(dest);
return *this;
}

/**
* Read a single 64-bit signed integer.
*
* @param [out] dest Where the 64-bit integer goes.
*
* @note The read can put the reader in a failed-status state if there are
* not enough octets available. Callers must either continue to do
* more reads on the return value or check its status to see whether
* the sequence of reads that has been performed succeeded.
*/
CHECK_RETURN_VALUE
Reader & ReadSigned64(int64_t * dest)
{
RawReadLowLevelBeCareful(dest);
return *this;
}

Expand All @@ -180,7 +270,7 @@ class Reader
* delegate to this one.
*/
template <typename T>
void RawRead(T * retval);
void RawReadLowLevelBeCareful(T * retval);

/**
* Advance the Reader forward by the specified number of octets.
Expand Down
20 changes: 20 additions & 0 deletions src/lib/support/BufferWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ LittleEndian::BufferWriter & LittleEndian::BufferWriter::EndianPut(uint64_t x, s
return *this;
}

LittleEndian::BufferWriter & LittleEndian::BufferWriter::EndianPutSigned(int64_t x, size_t size)
{
while (size > 0)
{
Put(static_cast<uint8_t>(x & 0xff));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be safer to just cast to uint64_t and then EndianPut that? That's obviously the reverse of what we do when reading in the BufferReader, whereas it's not entirely obvious without digging through C++ specs what the bitwise ops here will end up doing.

x >>= 8;
size--;
}
return *this;
}

BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPut(uint64_t x, size_t size)
{
while (size-- > 0)
Expand All @@ -69,5 +80,14 @@ BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPut(uint64_t x, size_t
return *this;
}

BigEndian::BufferWriter & BigEndian::BufferWriter::EndianPutSigned(int64_t x, size_t size)
{
while (size-- > 0)
{
Put(static_cast<uint8_t>((x >> (size * 8)) & 0xff));
}
return *this;
}

} // namespace Encoding
} // namespace chip
19 changes: 16 additions & 3 deletions src/lib/support/BufferWriter.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,18 @@ class EndianBufferWriterBase : public BufferWriter
Derived & Put(uint8_t c) { return static_cast<Derived &>(BufferWriter::Put(c)); }
Derived & Skip(size_t len) { return static_cast<Derived &>(BufferWriter::Skip(len)); }

// write an integer into a buffer, in an endian specific way
// write an integer into a buffer, in an endian-specific way

Derived & Put8(uint8_t c) { return static_cast<Derived *>(this)->Put(c); }
Derived & Put16(uint16_t x) { return static_cast<Derived *>(this)->EndianPut(x, sizeof(x)); }
Derived & Put32(uint32_t x) { return static_cast<Derived *>(this)->EndianPut(x, sizeof(x)); }
Derived & Put64(uint64_t x) { return static_cast<Derived *>(this)->EndianPut(x, sizeof(x)); }

Derived & PutSigned8(int8_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }
Derived & PutSigned16(int16_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }
Derived & PutSigned32(int32_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }
Derived & PutSigned64(int64_t x) { return static_cast<Derived *>(this)->EndianPutSigned(x, sizeof(x)); }

protected:
EndianBufferWriterBase(uint8_t * buf, size_t len) : BufferWriter(buf, len) {}
EndianBufferWriterBase(MutableByteSpan buf) : BufferWriter(buf.data(), buf.size()) {}
Expand All @@ -123,11 +128,15 @@ namespace LittleEndian {
class BufferWriter : public EndianBufferWriterBase<BufferWriter>
{
public:
BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len) {}
BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len)
{
static_assert((-1 & 3) == 3, "LittleEndian::BufferWriter only works with 2's complement architectures.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, would be good to document exactly where this dependency comes in. I think if you do cast to unsigned as suggested above there is no such dependency here (because cast to unsigned is well-defined in the C++ standard).

}
BufferWriter(MutableByteSpan buf) : EndianBufferWriterBase<BufferWriter>(buf) {}
BufferWriter(const BufferWriter & other) = default;
BufferWriter & operator=(const BufferWriter & other) = default;
BufferWriter & EndianPut(uint64_t x, size_t size);
BufferWriter & EndianPutSigned(int64_t x, size_t size);
};

} // namespace LittleEndian
Expand All @@ -137,11 +146,15 @@ namespace BigEndian {
class BufferWriter : public EndianBufferWriterBase<BufferWriter>
{
public:
BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len) {}
BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len)
{
static_assert((-1 & 3) == 3, "BigEndian::BufferWriter only works with 2's complement architectures.");
}
BufferWriter(MutableByteSpan buf) : EndianBufferWriterBase<BufferWriter>(buf) {}
BufferWriter(const BufferWriter & other) = default;
BufferWriter & operator=(const BufferWriter & other) = default;
BufferWriter & EndianPut(uint64_t x, size_t size);
BufferWriter & EndianPutSigned(int64_t x, size_t size);
};

} // namespace BigEndian
Expand Down
Loading