2:03 a.m.
Hi, Kazu
On Fri, May 27, 2022 at 2:35 PM <crash-utility-request(a)redhat.com> wrote:
Date: Fri, 27 May 2022 05:56:38 +0000
From: HAGIO KAZUHITO(?????) <k-hagio-ab(a)nec.com>
To: "crash-utility(a)redhat.com" <crash-utility(a)redhat.com>
Subject: Re: [Crash-utility] [PATCH] Fix for 'struct -o' option to
display the offsets of struct fields
Message-ID: <3b82e6a4-63db-d163-cc55-ec268c5a4b33(a)nec.com>
Content-Type: text/plain; charset="utf-8"
On 2022/05/27 11:36, HAGIO KAZUHITO(?? ??) wrote:
> On 2022/05/27 11:22, HAGIO KAZUHITO(?? ??) wrote:
>> Hi Lianbo,
>>
>> On 2022/05/26 16:07, Lianbo Jiang wrote:
>>> Currently, the 'struct -o' command does not display the offsets
>>> of struct fields like this:
>>>
>>> crash> struct page -ox ffffce98870a0d40
>>> struct page {
>>> [ffffce98870a0d40] unsigned long flags;
>>> union {
>>> struct {...};
>>> struct {...};
>>> struct {...};
>>> struct {...};
>>> struct {...};
>>> struct {...};
>>> struct {...};
>>> [ffffce98870a0d48] struct callback_head callback_head;
>>> };
>>> ...
>>> }
>>> SIZE: 0x40
>>
>> Good catch!
>>
>>>
>>> The gdb-10.2 added a new option '/o' for the 'ptype'
command, which
>>> prints the offsets and sizes of struct fields, let's use it now to
>>> fix the above issue.
>>
>> but isn't there another gdb option or setting to show their details?
>>
>> (gdb) ptype struct page
>> type = struct page {
>> unsigned long flags;
>> union {
>> struct {...};
>> struct {...};
>> struct {...};
>> ...
>>
>> If there is no setting other than '/o', it's a bit strange UI
design,
>> I think.. (although I cannot find so far..)
>>
Me too.
>>
>> Also the 'ptype /o' overdoes unfolding struct members,
>> and extra spaces and lines are printed with the patch:
>>
>> * crash-7.3.2
>>
>> crash-7.3.2> struct address_space
>> struct address_space {
>> struct inode *host;
>> struct xarray i_pages;
>> atomic_t i_mmap_writable;
>> struct rb_root_cached i_mmap;
>> struct rw_semaphore i_mmap_rwsem;
>> unsigned long nrpages;
>> ...
>> crash-7.3.2> struct -o address_space
>> struct address_space {
>> [0] struct inode *host;
>> [8] struct xarray i_pages;
>> [32] atomic_t i_mmap_writable;
>> [40] struct rb_root_cached i_mmap;
>> [56] struct rw_semaphore i_mmap_rwsem;
>> [96] unsigned long nrpages;
>>
>> * With the patch
>>
>> crash-dev> struct address_space
>> struct address_space {
>> struct inode *host;
>> struct xarray {
>> spinlock_t xa_lock;
>> gfp_t xa_flags;
>> void *xa_head;
>> size_t xarray_size_rh;
>> struct xarray_rh {
>> <no data fields>
>>
>>
>> } _rh;
>>
>>
>> } i_pages;
>> atomic_t i_mmap_writable;
>>
>> struct rb_root_cached {
>> struct rb_root {
>> struct rb_node *rb_node;
>> ...
>> crash-dev> struct -o address_space
>> struct address_space {
>> [0] struct inode *host;
>> struct xarray {
>> spinlock_t xa_lock;
>> gfp_t xa_flags;
>> void *xa_head;
>> size_t xarray_size_rh;
>> struct xarray_rh {
>> <no data fields>
>>
>>
>> } _rh;
>>
>>
>> [8] } i_pages;
>> [32] atomic_t i_mmap_writable;
>>
>> struct rb_root_cached {
>> struct rb_root {
>> struct rb_node *rb_node;
>> ...
>>
>>
>> task_struct also looks wrong:
>>
>> crash-dev> struct -o task_struct
>> struct task_struct {
>> struct thread_info {
>> [36] unsigned long flags;
>> u32 status;
>>
>>
>>
>> [0] } thread_info;
>> [16] volatile long state;
>> [24] void *stack;
>> ...
>>
>>
>> If no another gdb setting, we will need to rewrite the parser.
>> So first, I'd like to know whether there is no another setting.
>
> Otherwise, maybe we can patch the gdb code...
>
> That '{...}' is probably printed in c_type_print_base_struct_union().
> And found a comment for c_type_print_base_1() in gdb/c-typeprint.c:
>
> SHOW negative means just print the type name or struct tag if there
> is one. If there is no name, print something sensible but concise
> like "struct {...}".
>
> Just an idea and clue.
I like the good idea.
Just a rough try and I've not tested enough, but this patch might
be
somewhat good.
--- gdb-10.2.orig/gdb/c-typeprint.c 2022-05-27 14:49:53.079853333 +0900
+++ gdb-10.2/gdb/c-typeprint.c 2022-05-27 14:47:18.729165094 +0900
@@ -1043,6 +1043,8 @@
struct type_print_options local_flags = *flags;
local_flags.local_typedefs = NULL;
+ show = 1;
+
std::unique_ptr<typedef_hash_table> hash_holder;
if (!flags->raw)
{
This looks more reasonable to me. Could you please post a patch with this
fix?
Thanks.
Lianbo
Thanks,
Kazu
------------------------------
4:14 a.m.
New subject: [PATCH] Fix for 'struct -o' option to display the offsets of struct fields
On 2022/05/27 16:03, lijiang wrote:
>> If no another gdb setting, we will need to rewrite the
parser.
>> So first, I'd like to know whether there is no another setting.
>
> Otherwise, maybe we can patch the gdb code...
>
> That '{...}' is probably printed in c_type_print_base_struct_union().
> And found a comment for c_type_print_base_1() in gdb/c-typeprint.c:
>
> SHOW negative means just print the type name or struct tag if there
> is one. If there is no name, print something sensible but concise
> like "struct {...}".
>
> Just an idea and clue.
I like the good idea.
Just a rough try and I've not tested enough, but this patch might be
somewhat good.
--- gdb-10.2.orig/gdb/c-typeprint.c 2022-05-27 14:49:53.079853333 +0900
+++ gdb-10.2/gdb/c-typeprint.c 2022-05-27 14:47:18.729165094 +0900
@@ -1043,6 +1043,8 @@
struct type_print_options local_flags = *flags;
local_flags.local_typedefs = NULL;
+ show = 1;
+
std::unique_ptr<typedef_hash_table> hash_holder;
if (!flags->raw)
{
This looks more reasonable to me. Could you please post a patch with this fix?
Sure. But more understanding and testing are needed, I will look into it
next week.
Thanks,
Kazu
4:56 a.m.
New subject: [PATCH] Fix for 'struct -o' option to display the offsets of struct fields
On Fri, May 27, 2022 at 5:14 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
On 2022/05/27 16:03, lijiang wrote:
> >> If no another gdb setting, we will need to rewrite the parser.
> >> So first, I'd like to know whether there is no another setting.
> >
> > Otherwise, maybe we can patch the gdb code...
> >
> > That '{...}' is probably printed in
c_type_print_base_struct_union().
> > And found a comment for c_type_print_base_1() in
gdb/c-typeprint.c:
> >
> > SHOW negative means just print the type name or struct tag
if there
> > is one. If there is no name, print something sensible but
concise
> > like "struct {...}".
> >
> > Just an idea and clue.
>
>
> I like the good idea.
>
> Just a rough try and I've not tested enough, but this patch might be
> somewhat good.
>
> --- gdb-10.2.orig/gdb/c-typeprint.c 2022-05-27
14:49:53.079853333 +0900
> +++ gdb-10.2/gdb/c-typeprint.c 2022-05-27 14:47:18.729165094 +0900
> @@ -1043,6 +1043,8 @@
> struct type_print_options local_flags = *flags;
> local_flags.local_typedefs = NULL;
>
> + show = 1;
> +
> std::unique_ptr<typedef_hash_table> hash_holder;
> if (!flags->raw)
> {
>
>
> This looks more reasonable to me. Could you please post a patch with
this fix?
Sure. But more understanding and testing are needed, I will look into it
next week.
Sounds good and thank you for working on this issue, Kazu.
I will continue to improve the "dev -d" patchset.
Thanks.
Lianbo
Thanks,
Kazu
11:23 p.m.
New subject: [PATCH] Fix for 'struct -o' option to display the offsets of struct fields
Hi Lianbo,
On 2022/05/27 18:56, lijiang wrote:
On Fri, May 27, 2022 at 5:14 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com>> wrote:
On 2022/05/27 16:03, lijiang wrote:
> >> If no another gdb setting, we will need to rewrite the parser.
> >> So first, I'd like to know whether there is no another
setting.
> >
> > Otherwise, maybe we can patch the gdb code...
> >
> > That '{...}' is probably printed in
c_type_print_base_struct_union().
> > And found a comment for c_type_print_base_1() in gdb/c-typeprint.c:
> >
> > SHOW negative means just print the type name or struct tag if
there
> > is one. If there is no name, print something sensible but
concise
> > like "struct {...}".
> >
> > Just an idea and clue.
>
>
> I like the good idea.
>
> Just a rough try and I've not tested enough, but this patch might be
> somewhat good.
>
> --- gdb-10.2.orig/gdb/c-typeprint.c 2022-05-27 14:49:53.079853333
+0900
> +++ gdb-10.2/gdb/c-typeprint.c 2022-05-27 14:47:18.729165094 +0900
> @@ -1043,6 +1043,8 @@
> struct type_print_options local_flags = *flags;
> local_flags.local_typedefs = NULL;
>
> + show = 1;
> +
> std::unique_ptr<typedef_hash_table> hash_holder;
> if (!flags->raw)
> {
>
>
> This looks more reasonable to me. Could you please post a patch with this
fix?
Sure. But more understanding and testing are needed, I will look into it
next week.
I found that gdb-7.6 also prints "{...}" for unnamed struct/union.
(gdb) ptype struct page
type = struct page {
unsigned long flags;
struct address_space *mapping;
struct {
union {...};
union {...};
};
...
And crash-7.x (gdb-7.6.patch) had the following patch to print the details
of unnamed struct/union.
--- gdb-7.6/gdb/c-typeprint.c.orig
+++ gdb-7.6/gdb/c-typeprint.c
@@ -1097,7 +1097,8 @@ c_type_print_base (struct type *type, st
fprintf_filtered (stream, "static ");
c_print_type (TYPE_FIELD_TYPE (type, i),
TYPE_FIELD_NAME (type, i),
- stream, show - 1, level + 4,
+ stream, strlen(TYPE_FIELD_NAME (type, i)) ?
+ show - 1 : show, level + 4,
&local_flags);
if (!field_is_static (&TYPE_FIELD (type, i))
&& TYPE_FIELD_PACKED (type, i))
But gdb-10.2.patch does not have it, and prints "{...}" again.
So I will post the following patch later to do the same thing in gdb-10.2.
--- gdb-10.2/gdb/c-typeprint.c.orig 2022-05-31 13:15:39.099537173 +0900
+++ gdb-10.2/gdb/c-typeprint.c 2022-05-31 13:17:02.076909579 +0900
@@ -1202,6 +1202,9 @@ c_type_print_base_struct_union (struct t
= podata->end_bitpos
- TYPE_LENGTH (type->field (i).type ()) * TARGET_CHAR_BIT;
}
+ else if (strlen(TYPE_FIELD_NAME (type, i)) == 0)
+ /* crash: Print details for unnamed struct and union. */
+ newshow = show;
c_print_type_1 (type->field (i).type (),
TYPE_FIELD_NAME (type, i),
Thanks,
Kazu
1:18 a.m.
New subject: [PATCH] Fix for 'struct -o' option to display the offsets of struct fields
On Tue, May 31, 2022 at 12:23 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
Hi Lianbo,
On 2022/05/27 18:56, lijiang wrote:
> On Fri, May 27, 2022 at 5:14 PM HAGIO KAZUHITO(萩尾 一仁) <
k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com>> wrote:
>
> On 2022/05/27 16:03, lijiang wrote:
> > >> If no another gdb setting, we will need to rewrite the
parser.
> > >> So first, I'd like to know whether there is no another
setting.
> > >
> > > Otherwise, maybe we can patch the gdb code...
> > >
> > > That '{...}' is probably printed in
c_type_print_base_struct_union().
> > > And found a comment for c_type_print_base_1() in
gdb/c-typeprint.c:
> > >
> > > SHOW negative means just print the type name or
struct tag if there
> > > is one. If there is no name, print something
sensible but concise
> > > like "struct {...}".
> > >
> > > Just an idea and clue.
> >
> >
> > I like the good idea.
> >
> > Just a rough try and I've not tested enough, but this patch
might be
> > somewhat good.
> >
> > --- gdb-10.2.orig/gdb/c-typeprint.c 2022-05-27
14:49:53.079853333 +0900
> > +++ gdb-10.2/gdb/c-typeprint.c 2022-05-27 14:47:18.729165094
+0900
> > @@ -1043,6 +1043,8 @@
> > struct type_print_options local_flags = *flags;
> > local_flags.local_typedefs = NULL;
> >
> > + show = 1;
> > +
> > std::unique_ptr<typedef_hash_table> hash_holder;
> > if (!flags->raw)
> > {
> >
> >
> > This looks more reasonable to me. Could you please post a patch
with this fix?
>
> Sure. But more understanding and testing are needed, I will look
into it
> next week.
I found that gdb-7.6 also prints "{...}" for unnamed struct/union.
(gdb) ptype struct page
type = struct page {
unsigned long flags;
struct address_space *mapping;
struct {
union {...};
union {...};
};
...
And crash-7.x (gdb-7.6.patch) had the following patch to print the details
of unnamed struct/union.
--- gdb-7.6/gdb/c-typeprint.c.orig
+++ gdb-7.6/gdb/c-typeprint.c
@@ -1097,7 +1097,8 @@ c_type_print_base (struct type *type, st
fprintf_filtered (stream, "static ");
c_print_type (TYPE_FIELD_TYPE (type, i),
TYPE_FIELD_NAME (type, i),
- stream, show - 1, level + 4,
+ stream, strlen(TYPE_FIELD_NAME (type, i)) ?
+ show - 1 : show, level + 4,
&local_flags);
if (!field_is_static (&TYPE_FIELD (type, i))
&& TYPE_FIELD_PACKED (type, i))
But gdb-10.2.patch does not have it, and prints "{...}" again.
Good findings, Kazu.
So I will post the following patch later to do the same thing in
gdb-10.2.
--- gdb-10.2/gdb/c-typeprint.c.orig 2022-05-31 13:15:39.099537173 +0900
+++ gdb-10.2/gdb/c-typeprint.c 2022-05-31 13:17:02.076909579 +0900
@@ -1202,6 +1202,9 @@ c_type_print_base_struct_union (struct t
= podata->end_bitpos
- TYPE_LENGTH (type->field (i).type ()) *
TARGET_CHAR_BIT;
}
+ else if (strlen(TYPE_FIELD_NAME (type, i)) == 0)
+ /* crash: Print details for unnamed struct and union. */
+ newshow = show;
c_print_type_1 (type->field (i).type (),
TYPE_FIELD_NAME (type, i),
This looks good, it can keep the same behavior as gdb-7.6.
Thanks.
Lianbo
Thanks,
Kazu
3:15 a.m.
New subject: [PATCH] Fix for 'struct -o' option to display the offsets of struct fields
On 2022/05/31 15:18, lijiang wrote:
So I will post the following patch later to do the same thing in
gdb-10.2.
--- gdb-10.2/gdb/c-typeprint.c.orig 2022-05-31 13:15:39.099537173 +0900
+++ gdb-10.2/gdb/c-typeprint.c 2022-05-31 13:17:02.076909579 +0900
@@ -1202,6 +1202,9 @@ c_type_print_base_struct_union (struct t
= podata->end_bitpos
- TYPE_LENGTH (type->field (i).type ()) * TARGET_CHAR_BIT;
}
+ else if (strlen(TYPE_FIELD_NAME (type, i)) == 0)
+ /* crash: Print details for unnamed struct and union. */
+ newshow = show;
c_print_type_1 (type->field (i).type (),
TYPE_FIELD_NAME (type, i),
This looks good, it can keep the same behavior as gdb-7.6.
Thanks. The patch tested OK, I've posted.
Kazu
910
days inactive
914
days old
devel@lists.crash-utility.osci.io
5 comments
2 participants
participants (2)
-
HAGIO KAZUHITO(萩尾 一仁) -
lijiang