Re: [Crash-utility] [PATCH v1 0/3] Add valgrind support for the crash's custom memory
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>
> 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
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://listman.redhat.com/mailman/listinfo/crash-utility