[Crash-utility] [PATCH v1 0/3] Add valgrind support for the crash's custom memory

[lijiang]

在 2021年02月22日 09:12, crash-utility-request at redhat.com 写道:
> Date: Mon, 22 Feb 2021 00:37:36 +0000
> From: HAGIO KAZUHITO(?????) 	<k-hagio-ab at nec.com>
> To: "Discussion list for crash utility usage,	maintenance and
> 	development"	<crash-utility at redhat.com>
> Cc: "d.hatayama at fujitsu.com" <d.hatayama at fujitsu.com>
> Subject: Re: [Crash-utility] [PATCH v1 0/3] Add valgrind support for
> 	the	crash's custom memory
> Message-ID:
> 	<OSBPR01MB1991940E91708000BFEDE9D3DD819 at OSBPR01MB1991.jpnprd01.prod.outlook.com>
> 	
> Content-Type: text/plain; charset="utf-8"
> 
> Hi Lianbo, Hatayama-san,
> 
> -----Original Message-----
>> Hi, HATAYAMA
>>
>> ? 2021?01?21? 12:47, crash-utility-request at redhat.com ??:
>>> From: HATAYAMA Daisuke <d.hatayama at fujitsu.com>
>>> Sent: Monday, January 4, 2021 14:28
>>> To: crash-utility at redhat.com
>>> Cc: Hatayama, Daisuke/?? ??
>>> Subject: [PATCH v1 0/3] Add valgrind support for the crash's custom memory
>>>
>>> This patch set adds valgrind support for the crash's custom memory
>>> allocator. The motivation comes from investigation at
>>> https://github.com/crash-utility/crash-extensions/issues/1.
>>>
>>> The 1st patch implements the valgrind support, while 2nd and 3rd fixes
>>> the actual issues on the crash's custom memory allocator detected by
>>> valgrind.
>>>
>>> HATAYAMA Daisuke (3):
>>>   Add valgrind support for the crash's custom memory allocator
>>>   symbols: Fix potential read to already freed object
>>>   tools: Fix potential write to object of 0 size
>>>
>> Thank you for the patchset.
>>
>> This changes are fine to me, but I have some other concerns as below:
>>
>> [1] add a simple description for supporting 'make valgrind' in README?
> Good catch, thanks Lianbo.
> 
> FYI, note that the README data is also in help.c and needs to be updated.
> 
Sure.

>> [2] only pass the '-DVALGRIND' when using 'make valgrind' explicitly?
>>
>> For example:
>>
>> Step1: [root at dell-pet620-01 crash]# make valgrind
>> TARGET: X86_64
>>  CRASH: 7.2.9++
>>    GDB: 7.6
>>
>> cc -c -g -DX86_64 -DVALGRIND -DGDB_7_6  build_data.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes
>> -fstack-protector -Wformat-security
>> cc -c -g -DX86_64 -DVALGRIND -DGDB_7_6  tools.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes
>> -fstack-protector -Wformat-security
>> cc -c -g -DX86_64 -DVALGRIND -DGDB_7_6  global_data.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes
>> -fstack-protector -Wformat-security
>>                    ^^^^^^^^^
>> ...
>>
>> Step2: [root at dell-pet620-01 crash]# make lzo
>> TARGET: X86_64
>>  CRASH: 7.2.9++
>>    GDB: 7.6
>>
>> cc -c -g -DX86_64 -DVALGRIND -DLZO -DGDB_7_6  build_data.c -Wall -O2 -Wstrict-prototypes
>> -Wmissing-prototypes -fstack-protector -Wformat-security
>> cc -c -g -DX86_64 -DVALGRIND -DLZO -DGDB_7_6  main.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes
>> -fstack-protector -Wformat-security
>> cc -c -g -DX86_64 -DVALGRIND -DLZO -DGDB_7_6  tools.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes
>> -fstack-protector -Wformat-security
>>                    ^^^^^^^^^^^^^^^
>> ...
>>
>> For the 'make lzo', the cflag '-DVALGRIND' is also added here after the step1, is that expected?
> As written in the README, these targets are sticky, so it's expected:
> 
>   All of the alternate build commands above are "sticky" in that the
>   special "make" targets only have to be entered one time; all subsequent
>   builds will follow suit.
> 
> AFAIK, the only command that can drop a target is "make nowarn", otherwise
> we can drop "lzo" and so on by removing CFLAGS.extra and LDFLAGS.extra for
> the present.
> 
Seems yes. Is it possible to separate these CFLAGS? And we may put them together when
it is needed, For example:

make lzo           (-DLZO)
make valgrind      (-DVALGRIND)
make lzo_valgrind  (-DVALGRIND -DLZO)

But I'm not sure if it looks more reasonable. Anyway, this is another issue.

Thanks.
Lianbo

> Thanks,
> Kazu
> 
>>
>> BTW: this change could make the distribution add a new dependency of valgrind-devel package(A warm reminder).
>>
>> Thanks.
>> Lianbo
>>
>>>  Makefile    |  4 ++++
>>>  configure.c | 15 ++++++++++++-
>>>  symbols.c   | 10 +++------
>>>  tools.c     | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>  4 files changed, 91 insertions(+), 10 deletions(-)
>>>
>>> --
>>> 1.8.3.1