Skip to content

[Support] Add end/error to decode[US]LEB128AndInc #90006

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 8, 2024
Merged

[Support] Add end/error to decode[US]LEB128AndInc #90006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98181cf
[𝘀𝗽𝗿] initial version
MaskRay Apr 24, 2024
7d97a0c
make `end` mandatory and add `Unsafe` variant
MaskRay Apr 30, 2024
1218d5b
clang-format
MaskRay May 8, 2024
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
14 changes: 10 additions & 4 deletions llvm/include/llvm/Support/LEB128.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,20 +200,26 @@ inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
return Value;
}

inline uint64_t decodeULEB128AndInc(const uint8_t *&p) {
inline uint64_t decodeULEB128AndInc(const uint8_t *&p, const uint8_t *end,
const char **error = nullptr) {
unsigned n;
auto ret = decodeULEB128(p, &n);
auto ret = decodeULEB128(p, &n, end, error);
p += n;
return ret;
}

inline int64_t decodeSLEB128AndInc(const uint8_t *&p) {
inline int64_t decodeSLEB128AndInc(const uint8_t *&p, const uint8_t *end,
const char **error = nullptr) {
unsigned n;
auto ret = decodeSLEB128(p, &n);
auto ret = decodeSLEB128(p, &n, end, error);
p += n;
return ret;
}

inline uint64_t decodeULEB128AndIncUnsafe(const uint8_t *&p) {
return decodeULEB128AndInc(p, nullptr);
}

/// Utility function to get the size of the ULEB128-encoded value.
extern unsigned getULEB128Size(uint64_t Value);

Expand Down
25 changes: 24 additions & 1 deletion llvm/unittests/Support/LEB128Test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@ TEST(LEB128Test, DecodeInvalidULEB128) {
EXPECT_NE(Error, nullptr); \
EXPECT_EQ(0ul, Actual); \
EXPECT_EQ(ERROR_OFFSET, ErrorOffset); \
Value = reinterpret_cast<const uint8_t *>(VALUE); \
Error = nullptr; \
Actual = decodeULEB128AndInc(Value, Value + strlen(VALUE), &Error); \
EXPECT_NE(Error, nullptr); \
EXPECT_EQ(0ul, Actual); \
EXPECT_EQ(ERROR_OFFSET, Value - reinterpret_cast<const uint8_t *>(VALUE)); \
Copy link
Member

@aganea aganea May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hello @MaskRay!
There's no guarantee that VALUE here at L163 will be evaluated to the same value as VALUE when it was assigned at L158. These tests fail when building with latest MSVC using the Debug target. Same comment applies to other cases below.
Are you able to fix this or should I send a PR?

Copy link
Member

Choose a reason for hiding this comment

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

Note: Google Test filter = LEB128Test.DecodeInvalidULEB128
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from LEB128Test
[ RUN      ] LEB128Test.DecodeInvalidULEB128
C:\src\git\llvm-project\llvm\unittests\Support\LEB128Test.cpp(167): error: Expected equality of these values:
  0u
    Which is: 0
  Value - reinterpret_cast<const uint8_t *>("")
    Which is: -5

C:\src\git\llvm-project\llvm\unittests\Support\LEB128Test.cpp(168): error: Expected equality of these values:
  1u
    Which is: 1
  Value - reinterpret_cast<const uint8_t *>("\x80")
    Which is: -167

C:\src\git\llvm-project\llvm\unittests\Support\LEB128Test.cpp(171): error: Expected equality of these values:
  9u
    Which is: 9
  Value - reinterpret_cast<const uint8_t *>("\x80\x80\x80\x80\x80\x80\x80\x80\x80\x02")
    Which is: -167

C:\src\git\llvm-project\llvm\unittests\Support\LEB128Test.cpp(172): error: Expected equality of these values:
  10u
    Which is: 10
  Value - reinterpret_cast<const uint8_t *>("\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x02")
    Which is: -166

[  FAILED  ] LEB128Test.DecodeInvalidULEB128 (2 ms)
[----------] 1 test from LEB128Test (2 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (4 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] LEB128Test.DecodeInvalidULEB128

 1 FAILED TEST

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, I did not see the problem. If you have a PR, that will be appreciated!

} while (0)

// Buffer overflow.
Expand Down Expand Up @@ -224,6 +230,12 @@ TEST(LEB128Test, DecodeInvalidSLEB128) {
EXPECT_NE(Error, nullptr); \
EXPECT_EQ(0ul, Actual); \
EXPECT_EQ(ERROR_OFFSET, ErrorOffset); \
Value = reinterpret_cast<const uint8_t *>(VALUE); \
Error = nullptr; \
Actual = decodeSLEB128AndInc(Value, Value + strlen(VALUE), &Error); \
EXPECT_NE(Error, nullptr); \
EXPECT_EQ(0ul, Actual); \
EXPECT_EQ(ERROR_OFFSET, Value - reinterpret_cast<const uint8_t *>(VALUE)); \
} while (0)

// Buffer overflow.
Expand All @@ -246,7 +258,7 @@ TEST(LEB128Test, DecodeAndInc) {
#define EXPECT_LEB128(FUN, VALUE, SIZE) \
do { \
const uint8_t *V = reinterpret_cast<const uint8_t *>(VALUE), *P = V; \
auto Expected = FUN(P), Actual = FUN##AndInc(P); \
auto Expected = FUN(P), Actual = FUN##AndInc(P, P + strlen(VALUE)); \
EXPECT_EQ(Actual, Expected); \
EXPECT_EQ(P - V, SIZE); \
} while (0)
Expand All @@ -255,6 +267,17 @@ TEST(LEB128Test, DecodeAndInc) {
EXPECT_LEB128(decodeSLEB128, "\x7f", 1);
EXPECT_LEB128(decodeSLEB128, "\x80\x01", 2);
#undef EXPECT_LEB128

#define EXPECT_LEB128(FUN, VALUE, SIZE) \
do { \
const uint8_t *V = reinterpret_cast<const uint8_t *>(VALUE), *P = V; \
auto Expected = FUN(P), Actual = FUN##AndIncUnsafe(P); \
EXPECT_EQ(Actual, Expected); \
EXPECT_EQ(P - V, SIZE); \
} while (0)
EXPECT_LEB128(decodeULEB128, "\x7f", 1);
EXPECT_LEB128(decodeULEB128, "\x80\x01", 2);
#undef EXPECT_LEB128
}

TEST(LEB128Test, SLEB128Size) {
Expand Down
16 changes: 8 additions & 8 deletions llvm/utils/TableGen/DecoderEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2301,7 +2301,7 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
}
case MCD::OPC_CheckField: {
// Decode the start value.
unsigned Start = decodeULEB128AndInc(++Ptr);
unsigned Start = decodeULEB128AndIncUnsafe(++Ptr);
unsigned Len = *Ptr;)";
if (IsVarLenInst)
OS << "\n makeUp(insn, Start + Len);";
Expand All @@ -2328,7 +2328,7 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
}
case MCD::OPC_CheckPredicate: {
// Decode the Predicate Index value.
unsigned PIdx = decodeULEB128AndInc(++Ptr);
unsigned PIdx = decodeULEB128AndIncUnsafe(++Ptr);
// NumToSkip is a plain 24-bit integer.
unsigned NumToSkip = *Ptr++;
NumToSkip |= (*Ptr++) << 8;
Expand All @@ -2345,8 +2345,8 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
}
case MCD::OPC_Decode: {
// Decode the Opcode value.
unsigned Opc = decodeULEB128AndInc(++Ptr);
unsigned DecodeIdx = decodeULEB128AndInc(Ptr);
unsigned Opc = decodeULEB128AndIncUnsafe(++Ptr);
unsigned DecodeIdx = decodeULEB128AndIncUnsafe(Ptr);

MI.clear();
MI.setOpcode(Opc);
Expand All @@ -2366,8 +2366,8 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
}
case MCD::OPC_TryDecode: {
// Decode the Opcode value.
unsigned Opc = decodeULEB128AndInc(++Ptr);
unsigned DecodeIdx = decodeULEB128AndInc(Ptr);
unsigned Opc = decodeULEB128AndIncUnsafe(++Ptr);
unsigned DecodeIdx = decodeULEB128AndIncUnsafe(Ptr);
// NumToSkip is a plain 24-bit integer.
unsigned NumToSkip = *Ptr++;
NumToSkip |= (*Ptr++) << 8;
Expand Down Expand Up @@ -2399,8 +2399,8 @@ static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], MCInst &MI,
}
case MCD::OPC_SoftFail: {
// Decode the mask values.
uint64_t PositiveMask = decodeULEB128AndInc(++Ptr);
uint64_t NegativeMask = decodeULEB128AndInc(Ptr);
uint64_t PositiveMask = decodeULEB128AndIncUnsafe(++Ptr);
uint64_t NegativeMask = decodeULEB128AndIncUnsafe(Ptr);
bool Fail = (insn & PositiveMask) != 0 || (~insn & NegativeMask) != 0;
if (Fail)
S = MCDisassembler::SoftFail;
Expand Down