Hi, Kazu
On Fri, May 27, 2022 at 2:35 PM <crash-utility-request@xxxxxxxxxx> wrote:
Date: Fri, 27 May 2022 05:56:38 +0000
From: HAGIO KAZUHITO(?????) <k-hagio-ab@xxxxxxx>
To: "crash-utility@xxxxxxxxxx" <crash-utility@xxxxxxxxxx>
Subject: Re: [PATCH] Fix for 'struct -o' option to
display the offsets of struct fields
Message-ID: <3b82e6a4-63db-d163-cc55-ec268c5a4b33@xxxxxxx>
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
------------------------------
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
