Page MenuHomePhabricator

[lldb] [Process/FreeBSDRemote] Access GPR via reginfo offsets
ClosedPublic

Authored by mgorny on Tue, Nov 10, 3:17 PM.

Details

Summary

Read and write registers from m_gpr using offsets from RegisterInfo
rather than explicit switch-case. This eliminates a lot of redundant
code, and avoids mistakes such as type mismatches seen recently (wrt
segment registers). The same logic will be extended to other register
sets in the future.

Make m_gpr an uint8_t std::array to ease accesses. Ideally, we could
avoid including <machine/reg.h> entirely in the future and instead
get the correct GPR size from Utility/RegisterContextFreeBSD_* somehow.

While at it, modify register set logic to use an explicit enum with
llvm::Optional<>, making the code cleaner and at the same time enabling
compiler warnings for unhandled sets.

Since now we're fully relying on 'struct GPR' defined
in Utility/RegisterContextFreeBSD_* being entirely in sync with
the system structure, add unit tests to verify the field offsets
and sizes.

Diff Detail

Event Timeline

mgorny created this revision.Tue, Nov 10, 3:17 PM
mgorny requested review of this revision.Tue, Nov 10, 3:17 PM

Regarding the asserts, I'm thinking if it wouldn't be better to check the compare the resulting RegisterInfo structs instead of the raw struct layout. The reasons for that are:

  • it will be checking the thing that we're actually relying on (the register info macros is pretty complex and can be messed up easily
  • the register info artifacts (unlike the GPR structs) are exported from these files, so the checks for that could be placed into a unit test

What do you think?

Regarding the asserts, I'm thinking if it wouldn't be better to check the compare the resulting RegisterInfo structs instead of the raw struct layout. The reasons for that are:

  • it will be checking the thing that we're actually relying on (the register info macros is pretty complex and can be messed up easily
  • the register info artifacts (unlike the GPR structs) are exported from these files, so the checks for that could be placed into a unit test

What do you think?

I presume you mean verifying that byte_offsets and byte_sizes are written correctly to the struct? I suppose that makes sense.

I like the idea that static asserts are going to blow up during compilation already but I suppose it's not very important as I don't expect these structures to change.

Regarding the asserts, I'm thinking if it wouldn't be better to check the compare the resulting RegisterInfo structs instead of the raw struct layout. The reasons for that are:

  • it will be checking the thing that we're actually relying on (the register info macros is pretty complex and can be messed up easily
  • the register info artifacts (unlike the GPR structs) are exported from these files, so the checks for that could be placed into a unit test

What do you think?

I presume you mean verifying that byte_offsets and byte_sizes are written correctly to the struct?

Yes, exactly.

I like the idea that static asserts are going to blow up during compilation already but I suppose it's not very important as I don't expect these structures to change.

That's true, but I've found that the inclusion of system headers (with all their acoompanying ifdefs) looks pretty bad...

mgorny updated this revision to Diff 304433.Wed, Nov 11, 2:04 AM
mgorny edited the summary of this revision. (Show Details)

Updated to use unittests instead of static asserts.

mgorny updated this revision to Diff 304445.Wed, Nov 11, 2:59 AM

clang-format

mgorny updated this revision to Diff 304448.Wed, Nov 11, 3:11 AM

Add a helper ASSERT_REG macro.

I like this. Some comments in the tests -- trying to reduce the amount of preprocessor magic..

lldb/unittests/Process/FreeBSD/CMakeLists.txt
1 ↗(On Diff #304448)

I'd put this in unittests/Process/Utility, to match the location of the code being tested.

8–9 ↗(On Diff #304448)

Drop this, and include the file as "Plugins/Process/Utility/..."

lldb/unittests/Process/FreeBSD/RegisterContextFreeBSDTests.cpp
16 ↗(On Diff #304448)

These don't really have to be ifdefed, do they?

26–31 ↗(On Diff #304448)
pair<size_t(?), size_t> GetRegParams(const RegisterContext &ctx, unsigned reg) {
  const RegisterInfo &info = reg_ctx.GetRegisterInfo()[reg];
  return {info.byte_offset, info.byte_size};
}
35 ↗(On Diff #304448)
#define ASSERT_GPR_X86_64(regname)
  ASSERT_THAT(GetRegParams(reg_ctx, regname##_x86_64, testing::Pair(offsetof(reg, r_##regname), sizeof(reg::r_##regname))
72–74 ↗(On Diff #304448)

Inside the test function:

#ifdef __i386__
using native_i386_regs = ::reg32;
#else
using native_i386_regs = ::reg;
#endif
mgorny marked 6 inline comments as done.Thu, Nov 12, 6:00 AM
mgorny added inline comments.
lldb/unittests/Process/FreeBSD/RegisterContextFreeBSDTests.cpp
26–31 ↗(On Diff #304448)

Fun fact: RegisterContextFreeBSD_* is not a RegisterContext& but a RegisterInfoInterface& ;-).

mgorny updated this revision to Diff 304813.Thu, Nov 12, 6:04 AM

Implemented all the requested changes.

mgorny updated this revision to Diff 304859.Thu, Nov 12, 8:56 AM

clang-format

labath accepted this revision.Fri, Nov 13, 1:58 AM
labath added inline comments.
lldb/unittests/Process/FreeBSD/RegisterContextFreeBSDTests.cpp
26–31 ↗(On Diff #304448)

Yeah, tell me about it...

lldb/unittests/Process/Utility/CMakeLists.txt
2–3

Rename to ProcessUtilityTests -- all files in this folder should go into a single binary. We have more tests coming in D87442.

4

It seems we have some files ending in Tests, but the prevalent convention is just Test

lldb/unittests/Process/Utility/RegisterContextFreeBSDTests.cpp
38 ↗(On Diff #304859)

I guess the version is not really relevant for this..

This revision is now accepted and ready to land.Fri, Nov 13, 1:58 AM
labath added inline comments.Fri, Nov 13, 2:02 AM
lldb/unittests/Process/Utility/RegisterContextFreeBSDTests.cpp
32 ↗(On Diff #304859)

Also, please rename this to EXPECT_GRP_X86_64. ASSERT_ is for macros that terminate test when they fail.

mgorny marked 4 inline comments as done.Fri, Nov 13, 2:21 AM
mgorny updated this revision to Diff 305086.Fri, Nov 13, 3:50 AM

Implemented requested changes.

mgorny updated this revision to Diff 305371.Sun, Nov 15, 9:45 AM

Create the unit test unconditionally, guard the code with __FreeBSD__ define.

Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 16, 3:20 AM