Summary: | [Zephyr/thread aware debugging] remote: write_ptid returns negative tid. | ||
---|---|---|---|
Product: | gdb | Reporter: | Evgeniy <doubledn94> |
Component: | remote | Assignee: | Not yet assigned to anyone <unassigned> |
Status: | NEW --- | ||
Severity: | normal | CC: | fredrik.hederstierna, obilaniu, simark, tromey |
Priority: | P2 | ||
Version: | 8.3.1 | ||
Target Milestone: | --- | ||
Host: | Target: | ||
Build: | Last reconfirmed: | 2022-01-07 00:00:00 | |
Attachments: | write_ptid_long_type.patch |
Description
Evgeniy
2019-10-17 07:24:37 UTC
When looking into Zephyr and OpenOCD I just saw this ticket. Isn't there a discrepancy in <gdb/remote.c> for ptid_t write/read when it comes to types. The 'lwp' is used as 'tid', but has a wider type: int ptid_get_pid (ptid_t ptid) long ptid_get_lwp (ptid_t ptid) long ptid_get_tid (ptid_t ptid) so in the <read_ptid> it reads into ULONGEST, which I guess is uint64_t, but the <write_tpid> instead implicit cast long to int when using 'lwp as 'tid' (which I believe is often int32_t on a normal host target machine): int tid; tid = ptid.lwp (); Wouldn't it be better to keep the wider long type, and keep calling 'tid' for 'lwp' which it actually is? And also keeping the long-type in the write function? In this case there won't be any overflow if an 32bit int representing 'tid' is becoming >0x80000000. The read function I guess will still handle it as it is using ULONGEST to my understanding. Attaching idea of a possible patch. Created attachment 13765 [details]
write_ptid_long_type.patch
Extends type of 'tid' when writing ptid.
At first glance, it seems you're right. *** Bug 14314 has been marked as a duplicate of this bug. *** Can you send the patch to gdb-patches? See the contribution checklist for all the details: https://sourceware.org/gdb/wiki/ContributionChecklist I think this is a longstanding bug, see the dup I just closed... I guess that never happened, the bug is still there. It seems weird to me that we used signed types in ptid_t. Unfortunately I am currently not using Zephyr myself, so I would probably need some help to get a better formatting and testing the patch. Anyone is welcome to assist in improving the packaging/testing the patch, if interested in getting this patch merged. Unfortunately I haven't any good Zephyr test setup myself. I looked a little. Changing ptid_t::pid_type to be unsigned is probably a colossal change, bordering on the impossible. However, it does seem that negative values aren't really intended -- except for the special case of -1. The manual says: The PID (process) and TID (thread) components each have the format described above: a positive number with target-specific interpretation formatted as a big-endian hex string, literal ‘-1’ to indicate all processes or threads (respectively), or ‘0’ to indicate an arbitrary process or thread. So in remote_target::write_ptid, I wonder if it's sufficient to change the "pid < 0" check to "pid == -1" (and likewise for tid). |