[JITLink][AArch32] Unittest for error paths of readAddend and applyFixup functionality #69636
Conversation
eymay
force-pushed
the
cov_test_readAddend
branch
from
6bad67f to
ee5cbe9
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Nice! Testing error paths sounds like a good addition and it might be something to consider even in a broader scope.
The current implementation looks quite generic though. It's hard to see what is the actual error messages we expect. Maybe we can improve that with matchers. Please see my inline comment for an example. It might even allow to reduce most of the createXYError() boilderplate.
Edit: And can we move it into a separate AArch32ErrorTests.cpp maybe?
What do you think?
eymay
force-pushed
the
cov_test_readAddend
branch
from
ee5cbe9 to
a242afe
Compare
eymay
changed the title
eymay
force-pushed
the
cov_test_readAddend
branch
from
75a795b to
5ff858d
Compare
eymay
force-pushed
the
cov_test_readAddend
branch
from
5ff858d to
c9eb0ce
Compare
There was a problem hiding this comment.
Thanks! Our opcode checks are not exhaustive. It's rather smoke checks. The goal is to raise a flag when we get code that we cannot handle yet. Up to you to fix the Jump24 constant here or in a separate PR (of leave it to me if you wish). Two comments with more detail inline:
| static constexpr HalfWords Opcode{0xf000, 0x8000}; | ||
| static constexpr HalfWords OpcodeMask{0xf800, 0x8000}; | ||
| static constexpr HalfWords Opcode{0xf000, 0x9000}; | ||
| static constexpr HalfWords OpcodeMask{0xf800, 0xd000}; |
There was a problem hiding this comment.
The docs here state that Jump24 requires a B.W instruction. The old opcode and mask match both of its encodings: B-T3 and B-T4. We only support B-T4 right now (and I don't think Jump24 can even be applied to the other one). So indeed, we should fix both, opcode and mask, to be 0x9000.
| static constexpr HalfWords Opcode{0xf000, 0xc000}; | ||
| static constexpr HalfWords OpcodeMask{0xf800, 0xc000}; | ||
| static constexpr HalfWords Opcode{0xf000, 0xd000}; | ||
| static constexpr HalfWords OpcodeMask{0xf800, 0xd000}; |
There was a problem hiding this comment.
Call opcode and mask should both remain at 0xC000 because the input object can come with BL-T1 or BLX-T2, so we don't want to check bit0 of the most significant byte in Lo.
eymay
force-pushed
the
cov_test_readAddend
branch
from
c9eb0ce to
5ad2171
Compare
| EXPECT_THAT_EXPECTED(readAddendThumb(*G, BArm, InvalidEdge, ArmCfg), | ||
| FailedWithMessage(testing::HasSubstr( | ||
| "can not read implicit addend for aarch32 edge kind " | ||
| "INVALID RELOCATION"))); |
There was a problem hiding this comment.
Can we deduplicate these across all test cases please?
This commit is part of an effort to increase coverage of AArch32 backend.
eymay
force-pushed
the
cov_test_readAddend
branch
from
5ad2171 to
563d5e0
Compare
This commit is part of an effort to increase coverage of AArch32 backend.