Page MenuHomePhabricator

[lldb] Correct format of qMemTags type field
ClosedPublic

Authored by DavidSpickett on Jun 25 2021, 7:00 AM.

Details

Summary

The type field is a signed integer.
(https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html)

However it's not packed in the packet in the way
you might think. For example the type -1 should be:
qMemTags:<addr>,<len>:ffffffff
Instead of:
qMemTags:<addr>,<len>:-1

This change makes lldb-server's parsing more strict
and adds more tests to check that we handle negative types
correctly in lldb and lldb-server.

We only support one tag type value at this point,
for AArch64 MTE, which is positive. So this doesn't change
any of those interactions. It just brings us in line with GDB.

Also check that the test target has MTE. Previously
we just checked that we were AArch64 with a toolchain
that supports MTE.

Finally, update the tag type check for QMemTags to use
the same conversion steps that qMemTags now does.
Using static_cast can invoke UB and though we do do a limit
check to avoid this, I think it's clearer with the new method.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jun 25 2021, 7:00 AM
DavidSpickett requested review of this revision.Jun 25 2021, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 7:00 AM
omjavaid added inline comments.Jun 28 2021, 4:37 PM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3489

First condition looks redundant given that anything above 32 bit range is invalid and being tested in next condition?

omjavaid added inline comments.Jun 29 2021, 4:03 PM
lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
30–31

Missing isAArch64MTE check here?

  • Remove redundant check on type value
  • Explain how we check the target has MTE
DavidSpickett marked 2 inline comments as done.Jun 30 2021, 2:40 AM

Any other comments?

lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py
30–31

Added a comment to explain this.

omjavaid accepted this revision.Jul 12 2021, 2:14 AM

I am trying to run test_qmemtags_packets but the test hangs while waiting for lldb-server to terminate.

My test compiler is gcc-linaro-11.0.0-2021.02-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc (Monthly Pre built binaries uploaded by linaro)

using commandline lldb-dotest -A aarch64 -C compiler --platform-name=remote-linux --platform-url=<QEMU VM URL>

This revision is now accepted and ready to land.Jul 12 2021, 2:14 AM
omjavaid requested changes to this revision.Jul 12 2021, 2:15 AM
This revision now requires changes to proceed.Jul 12 2021, 2:15 AM

I am trying to run test_qmemtags_packets but the test hangs while waiting for lldb-server to terminate.

Do you have any more detail than that? I presume it runs the test then hangs during the cleanup. Without this patch do you see the same thing?

I'm using Kernel 5.14-rc1, Qemu 6.0.0 and a gcc 10.2 I built with crosstool NG (because of some glibc version issues). The compiler could be the difference.

  • Rebase onto main
  • Update the way we cast down to int32_t to avoid use of static_cast.

We weren't invoking the undefined behaviour of unsigned to signed
due to the limit check. However I think this shows our intent more clearly.

It's essentially C++20's std::bitcast.

Also I was unable to reproduce the test failures,
ok to land this as is given that QMemTags has similair tests
and is already on main?

DavidSpickett edited the summary of this revision. (Show Details)Jul 28 2021, 7:00 AM
omjavaid accepted this revision.Jul 30 2021, 2:19 AM
This revision is now accepted and ready to land.Jul 30 2021, 2:19 AM
This revision was landed with ongoing or failed builds.Jul 30 2021, 3:07 AM
This revision was automatically updated to reflect the committed changes.