From: Antoine Tremblay <antoine.tremblay@ericsson.com>
To: "Marcin Kościelnicki" <koriakin@0x04.net>,
"Pedro Alves" <palves@redhat.com>,
gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.
Date: Fri, 29 Jan 2016 13:11:00 -0000 [thread overview]
Message-ID: <56AB650B.1000204@ericsson.com> (raw)
In-Reply-To: <56A61255.8020503@0x04.net>
On 01/25/2016 07:17 AM, Marcin Kościelnicki wrote:
> On 25/01/16 12:53, Pedro Alves wrote:
>> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
>>> The struct tracepoint_action has an ops field, pointing to
>>> a tracepoint_action_ops structure, containing send and download ops.
>>> However, this field is only present when compiled in gdbserver, and not
>>> when compiled in IPA. When gdbserver is downloading tracepoint actions
>>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
>>> its struct tracepoint_action, to get to the part that corresponds to
>>> IPA's struct tracepoint_action.
>>>
>>> Unfortunately, this fails badly on ILP32 platforms where alignof(long
>>> long)
>>> == 8. Consider struct collect_memory_action layout in gdbserver:
>>>
>>> 0-3: base.ops
>>> 4: base.type
>>> 8-15: addr
>>> 16-23: len
>>> 24-27: basereg
>>> sizeof == 32
>>>
>>> and its layout in IPA:
>>>
>>> 0: base.type
>>> 8-15: addr
>>> 16-23: len
>>> 24-27: basereg
>>> sizeof == 32
>>>
>>> When gdbserver tries to download it to IPA, it skips the first 4 bytes
>>> (base.ops), figuring the rest will match what IPA expects - which is
>>> not true, since addr is aligned to 8 bytes and will be at a different
>>> relative position to base.type.
>>>
>>> The problem went unnoticed on the currently supported platforms, since
>>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
>>> alignment for long long.
>>>
>>> There are a few possible ways around this problem. I decided on
>>> removing
>>> ops altogether, since they can be easily inlined in their (only) places
>>> of use - in fact allowing us share the code between 'L' and 'R'. Any
>>> approach where struct tracepoint_action is different between IPA and
>>> gdbserver is just asking for trouble.
>>>
>>> Found on s390. Tested on x86_64, s390, s390x.
>>
>> Hmm, this is essentially the same as:
>>
>> https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html
>>
>> Right?
>>
>> Seems that other patch inlines things a bit less though, which offhand
>> looks preferable. WDYT?
>>
>> Not sure what happened to that series. I thought most of it (if not all)
>> had been approved already.
>>
>> Thanks,
>> Pedro Alves
>>
>
> Huh, I didn't know about that patch series. Good to know, since I was
> going to try doing ppc tracepoints next, and had no idea that has
> already been attempted. What happened to that patchset/author? Kind of
> strange to abandon mostly-finished code when there's a $3k bounty on it.
>
> The other patch looks fine too, I have no preference here.
>
> Marcin Kościelnicki
I had the same problem on ARM.
We could just keep the struct and pack it too, this is common for ABIs
IMO...
It would avoid this kind of mistake in the future if we were going to
reintroduce something similar...
I had this patch in the pipeline:
Author: Antoine Tremblay <antoine.tremblay@ericsson.com>
Date: Thu Jan 28 13:03:10 2016 -0500
Fix structure alignment problems with IPA protocol objects.
While testing fast tracepoints on ARM I came across this problem :
Program received signal SIGSEGV, Segmentation fault.
0x4010b56c in ?? () from target:/lib/arm-linux-gnueabihf/libc.so.6
(gdb) FAIL: gdb.trace/ftrace.exp: ond globvar > 7: continue
The problem is that on GDBServer side struct tracepoint_action is
aligned
on 4 bytes, and collect_memory_action is aligned on 8. However in
the IPA
they are both aligned on 8 bytes.
Thus when we copy the data from GDBServer's struct
tracepoint_action->type
offset to the ipa the alignement is wrong and the addr,len,basereg
values
are wrong also, causing a crash in the inferiror as it tries to read
memory at a bogus address.
This patch fixes this issue by setting the tracepoint_action and
collect_memory_action as packed.
This patch also fixes this issue in general by setting all IPA protocol
object structures as packed.
gdb/gdbserver/ChangeLog:
* tracepoint.c (ATTR_PACKED): Moved earlier in the file.
(struct tracepoint_action): Use ATTR_PACKED.
(struct collect_memory_action): Likewise.
(struct collect_registers_action): Likewise.
(struct eval_expr_action): Likewise.
(struct collect_static_trace_data_action): Likewise.
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 33f6120..35ce2ad 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -467,6 +467,14 @@ static int write_inferior_data_ptr (CORE_ADDR
where, CORE_ADDR ptr);
#endif
+#ifndef ATTR_PACKED
+# if defined(__GNUC__)
+# define ATTR_PACKED __attribute__ ((packed))
+# else
+# define ATTR_PACKED /* nothing */
+# endif
+#endif
+
/* Operations on various types of tracepoint actions. */
struct tracepoint_action;
@@ -490,7 +498,7 @@ struct tracepoint_action
const struct tracepoint_action_ops *ops;
#endif
char type;
-};
+} ATTR_PACKED;
/* An 'M' (collect memory) action. */
struct collect_memory_action
@@ -500,14 +508,14 @@ struct collect_memory_action
ULONGEST addr;
ULONGEST len;
int32_t basereg;
-};
+} ATTR_PACKED;
/* An 'R' (collect registers) action. */
struct collect_registers_action
{
struct tracepoint_action base;
-};
+} ATTR_PACKED;
/* An 'X' (evaluate expression) action. */
@@ -516,13 +524,13 @@ struct eval_expr_action
struct tracepoint_action base;
struct agent_expr *expr;
-};
+} ATTR_PACKED;
/* An 'L' (collect static trace data) action. */
struct collect_static_trace_data_action
{
struct tracepoint_action base;
-};
+} ATTR_PACKED;
#ifndef IN_PROCESS_AGENT
static CORE_ADDR
@@ -828,14 +836,6 @@ IP_AGENT_EXPORT_VAR struct trace_state_variable
*trace_state_variables;
when the wrapped-around trace frame is the one being discarded; the
free space ends up in two parts at opposite ends of the buffer. */
-#ifndef ATTR_PACKED
-# if defined(__GNUC__)
-# define ATTR_PACKED __attribute__ ((packed))
-# else
-# define ATTR_PACKED /* nothing */
-# endif
-#endif
-
/* The data collected at a tracepoint hit. This object should be as
small as possible, since there may be a great many of them. We do
not need to keep a frame number, because they are all sequential
WDYT ?
Thanks,
Antoine
next prev parent reply other threads:[~2016-01-29 13:11 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-23 19:32 Marcin Kościelnicki
2016-01-25 11:54 ` Pedro Alves
2016-01-25 12:17 ` Marcin Kościelnicki
2016-01-29 13:11 ` Antoine Tremblay [this message]
2016-02-08 11:55 ` Marcin Kościelnicki
2016-02-08 12:54 ` Antoine Tremblay
2016-01-29 10:14 ` Marcin Kościelnicki
2016-02-06 1:05 ` Marcin Kościelnicki
2016-02-08 13:05 ` Antoine Tremblay
2016-02-11 16:30 ` [PATCH] gdbserver: Remove tracepoint_action ops Marcin Kościelnicki
2016-02-11 16:50 ` Pedro Alves
2016-02-11 22:24 ` Marcin Kościelnicki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56AB650B.1000204@ericsson.com \
--to=antoine.tremblay@ericsson.com \
--cc=gdb-patches@sourceware.org \
--cc=koriakin@0x04.net \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).