Date: Mon, 22 Feb 2021 00:37:36 +0000 From: HAGIO KAZUHITO(?????) <k-hagio-ab(a)nec.com&gt; To: "Discussion list for crash utility usage, maintenance and development" <crash-utility(a)redhat.com&gt; Cc: &quot;d.hatayama(a)fujitsu.com&quot; <d.hatayama(a)fujitsu.com&gt; Subject: Re: [Crash-utility] [PATCH v1 0/3] Add valgrind support for the crash's custom memory Message-ID: <OSBPR01MB1991940E91708000BFEDE9D3DD819(a)OSBPR01MB1991.jpnprd01.prod.outlook.com&gt; Content-Type: text/plain; charset="utf-8" Hi Lianbo, Hatayama-san, -----Original Message----- > Hi, HATAYAMA > > ? 2021?01?21? 12:47, crash-utility-request(a)redhat.com ??: >> From: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com&gt; >> Sent: Monday, January 4, 2021 14:28 >> To: crash-utility(a)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.

> [2] only pass the '-DVALGRIND' when using 'make valgrind' explicitly? > > For example: > > Step1: [root@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@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.

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