From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
To: gdb-patches@sourceware.org
Subject: Re: [PATCH] sim/m32c: fix memory leaks in opc2c
Date: Wed, 7 Apr 2021 10:19:44 -0400 [thread overview]
Message-ID: <84f2de14-6806-2cbc-0657-c64fe6a5f936@polymtl.ca> (raw)
In-Reply-To: <YG2ZrOjLVT7TLR9d@vapier>
On 2021-04-07 7:38 a.m., Mike Frysinger wrote:
> On 06 Apr 2021 21:45, Simon Marchi via Gdb-patches wrote:
>>> i'm not keen on pushing it in this direction exactly. it would mean every
>>> caller would have to update & keep track.
>>>
>>> i think you could define an IGEN variable in common/Make-common.in and change
>>> all callers over to that. then that would be the only place you'd have to add
>>> any sanitizer related variables to.
>>
>> I can do that, but I'm not sure where in common/Make-common.in I should
>> add the variable. There seems to be a logic to the organization in that
>> file, but I don't get it.
>
> *shrug* there isn't much to it. you can put it after the POSTCOMPILE= line.
>
>> Also, not that ppc has its own igen, so I
>> guess it will still use its own definition.
>
> correct, for now, it's duplicated and we eat that cost
>
>> Another option is to just factor out the env var:
>>
>> DISABLE_LSAN = ASAN_OPTIONS=detect_leaks=0
>>
>> ... and still use it in rules:
>>
>> target: source
>> $(DISABLE_LSAN) ../igen/igen --blah
>
> i'm not keen on doing sanitizer-specific vars. there's ASAN, LSAN, TSAN,
> UBSAN, KSAN, and prob more in the future. that's why having a common IGEN
> var and then having it use a generic name (SANITIZE_ENV?) would work best
> imo.
>
>> Side-note, I saw some `@GMAKE_TRUE@` in Make-common.in. In GDB, we
>> decided to require GNU make and remove that complexity, in case you want
>> to do the same.
>
> sim/ is changing to automake, so a lot of that stuff will go away entirely.
> i'm focusing on that rather than chipping away at smaller bits.
>
> you can see this with the igen/ dir:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=b6b1c790843087e67e85e7cfd3327a872c03c6bc
>
> although i need to do more groundwork in the C side first.
> -mike
>
See updated patch below.
From 1d03bd9b0173a319eddab82d41bfb1671bfcc82d Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Tue, 6 Apr 2021 08:54:00 -0400
Subject: [PATCH] sim: set ASAN_OPTIONS=detect_leaks=0 when running igen and
opc2c
The igen/dgen and opc2c tools leak their heap-allocated memory (on
purpose) at program exit, which makes AddressSanitizer fail the tool
execution. This breaks the build, as it makes the tool return a
non-zero exit code.
Fix that by disabling leak detection through the setting of that
environment variable.
I also changed the opc2c rules for m32c to go through a temporary file.
What happened is that the failing opc2c would produce an incomplete file
(probably because ASan exits the process before stdout is flushed).
This meant that further make attempts didn't try to re-create the file,
as it already existed. A "clean" was therefore necessary. This can
also happen in regular builds if the user interrupts the build (^C) in
the middle of the opc2c execution and tries to resume it. Going to a
temporary file avoids this issue.
sim/m32c/ChangeLog:
* Makefile.in: Set ASAN_OPTIONS when running opc2c.
sim/mips/ChangeLog:
* Makefile.in: Set ASAN_OPTIONS when running igen.
sim/mn10300/ChangeLog:
* Makefile.in: Set ASAN_OPTIONS when running igen.
sim/ppc/ChangeLog:
* Makefile.in: Set ASAN_OPTIONS when running igen.
sim/v850/ChangeLog:
* Makefile.in: Set ASAN_OPTIONS when running igen.
Change-Id: I00f21d4dc1aff0ef73471925d41ce7c23e83e082
---
sim/common/Make-common.in | 4 ++++
sim/m32c/Makefile.in | 10 ++++++++--
sim/mips/Makefile.in | 20 ++++++++++----------
sim/mn10300/Makefile.in | 2 +-
sim/ppc/Makefile.in | 10 ++++++++--
sim/v850/Makefile.in | 2 +-
6 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/sim/common/Make-common.in b/sim/common/Make-common.in
index c8445bce59ea..28f50abb2205 100644
--- a/sim/common/Make-common.in
+++ b/sim/common/Make-common.in
@@ -111,6 +111,10 @@ COMPILE.post = -c -o $@
COMPILE = $(COMPILE.pre) $(ALL_CFLAGS) $(COMPILE.post)
POSTCOMPILE = @true
+# igen leaks memory, and therefore makes AddressSanitizer unhappy. Disable
+# leak detection while running it.
+IGEN = ASAN_OPTIONS=detect_leaks=0 ../igen/igen
+
# Each simulator's Makefile.in defines one or more of these variables
# to override our settings as necessary. There is no need to define these
# in the simulator's Makefile.in if one is using the default value. In fact
diff --git a/sim/m32c/Makefile.in b/sim/m32c/Makefile.in
index 6bc5c5b743d9..186c9c063d32 100644
--- a/sim/m32c/Makefile.in
+++ b/sim/m32c/Makefile.in
@@ -46,11 +46,17 @@ LIBS = $B/bfd/libbfd.a $B/libiberty/libiberty.a
arch = m32c
+# opc2c leaks memory, and therefore makes AddressSanitizer unhappy. Disable
+# leak detection while running it.
+OPC2C = ASAN_OPTIONS=detect_leaks=0 ./opc2c
+
r8c.c : r8c.opc opc2c
- ./opc2c -l r8c.out $(srcdir)/r8c.opc > r8c.c
+ $(OPC2C) -l r8c.out $(srcdir)/r8c.opc > r8c.c.tmp
+ mv r8c.c.tmp r8c.c
m32c.c : m32c.opc opc2c
- ./opc2c -l m32c.out $(srcdir)/m32c.opc > m32c.c
+ $(OPC2C) -l m32c.out $(srcdir)/m32c.opc > m32c.c.tmp
+ mv m32c.c.tmp m32c.c
opc2c : opc2c.o safe-fgets.o
$(LINK_FOR_BUILD) $^
diff --git a/sim/mips/Makefile.in b/sim/mips/Makefile.in
index c66c6e839782..00906451c4cc 100644
--- a/sim/mips/Makefile.in
+++ b/sim/mips/Makefile.in
@@ -149,7 +149,7 @@ BUILT_SRC_FROM_IGEN = \
$(BUILT_SRC_FROM_IGEN): tmp-igen
tmp-igen: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -220,7 +220,7 @@ BUILT_SRC_FROM_M16 = \
$(BUILT_SRC_FROM_M16): tmp-m16
tmp-m16: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -255,7 +255,7 @@ tmp-m16: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
$(SHELL) $(srcdir)/../../move-if-change tmp-model.c m16_model.c
$(SHELL) $(srcdir)/../../move-if-change tmp-support.h m16_support.h
$(SHELL) $(srcdir)/../../move-if-change tmp-support.c m16_support.c
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -292,7 +292,7 @@ tmp-m16: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
$(SHELL) $(srcdir)/../../move-if-change tmp-model.c m32_model.c
$(SHELL) $(srcdir)/../../move-if-change tmp-support.h m32_support.h
$(SHELL) $(srcdir)/../../move-if-change tmp-support.c m32_support.c
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -346,7 +346,7 @@ BUILT_SRC_FROM_MICROMIPS = \
$(BUILT_SRC_FROM_MICROMIPS): tmp-micromips
tmp-micromips: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -391,7 +391,7 @@ tmp-micromips: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
micromips16_support.h
$(SHELL) $(srcdir)/../../move-if-change tmp-support.c \
micromips16_support.c
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -436,7 +436,7 @@ tmp-micromips: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
micromips32_support.h
$(SHELL) $(srcdir)/../../move-if-change tmp-support.c \
micromips32_support.c
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -481,7 +481,7 @@ tmp-micromips: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
micromips_m32_support.h
$(SHELL) $(srcdir)/../../move-if-change tmp-support.c \
micromips_m32_support.c
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
@@ -521,7 +521,7 @@ tmp-mach-multi: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
m16*) e="-B 16 -H 15 -o $(M16_DC) -F 16" ;; \
*) e="-B 32 -H 31 -o $(IGEN_DC) -F $${f}" ;; \
esac; \
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
$${e} \
-I $(srcdir) \
@@ -574,7 +574,7 @@ tmp-mach-multi: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
done
touch tmp-mach-multi
tmp-itable-multi: $(IGEN_INSN) $(IGEN_DC) ../igen/igen $(IGEN_INCLUDE)
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-I $(srcdir) \
-Werror \
diff --git a/sim/mn10300/Makefile.in b/sim/mn10300/Makefile.in
index 6cdf9712882b..773b7f9a0c59 100644
--- a/sim/mn10300/Makefile.in
+++ b/sim/mn10300/Makefile.in
@@ -69,7 +69,7 @@ IGEN_TRACE= # -G omit-line-numbers # -G trace-rule-selection -G trace-rule-rejec
IGEN_INSN=$(srcdir)/mn10300.igen $(srcdir)/am33.igen $(srcdir)/am33-2.igen
IGEN_DC=$(srcdir)/mn10300.dc
tmp-igen: $(IGEN_INSN) $(IGEN_DC) ../igen/igen
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-G gen-direct-access \
-M mn10300,am33 -G gen-multi-sim=am33 \
diff --git a/sim/ppc/Makefile.in b/sim/ppc/Makefile.in
index d2bd1d317754..d9d01985404b 100644
--- a/sim/ppc/Makefile.in
+++ b/sim/ppc/Makefile.in
@@ -133,6 +133,12 @@ IGEN_FLAGS = \
$(IGEN_SMP) \
$(IGEN_LINE_NR)
+# igen/dgen leak memory, and therefore makes AddressSanitizer unhappy. Disable
+# leak detection while running them.
+
+IGEN = ASAN_OPTIONS=detect_leaks=0 ./igen
+DGEN = ASAN_OPTIONS=detect_leaks=0 ./dgen
+
.NOEXPORT:
MAKEOVERRIDES=
@@ -666,7 +672,7 @@ ppc-config.h: $(CONFIG_FILE)
tmp-dgen: dgen ppc-spr-table $(srcdir)/../../move-if-change
- ./dgen $(DGEN_FLAGS) \
+ $(DGEN) $(DGEN_FLAGS) \
-r $(srcdir)/ppc-spr-table \
-n spreg.h -hp tmp-spreg.h \
-n spreg.c -p tmp-spreg.c
@@ -675,7 +681,7 @@ tmp-dgen: dgen ppc-spr-table $(srcdir)/../../move-if-change
touch tmp-dgen
tmp-igen: igen $(srcdir)/ppc-instructions $(srcdir)/altivec.igen $(srcdir)/e500.igen $(IGEN_OPCODE_RULES) $(srcdir)/../../move-if-change tmp-ld-decode tmp-ld-cache tmp-ld-insn tmp-filter
- ./igen $(IGEN_FLAGS) \
+ $(IGEN) $(IGEN_FLAGS) \
-o $(srcdir)/$(IGEN_OPCODE_RULES) \
-I $(srcdir) -i $(srcdir)/ppc-instructions \
-n icache.h -hc tmp-icache.h \
diff --git a/sim/v850/Makefile.in b/sim/v850/Makefile.in
index 983fc79f93a7..250129c549f7 100644
--- a/sim/v850/Makefile.in
+++ b/sim/v850/Makefile.in
@@ -69,7 +69,7 @@ IGEN_TRACE= # -G omit-line-numbers # -G trace-rule-selection -G trace-rule-rejec
IGEN_INSN=$(srcdir)/v850.igen
IGEN_DC=$(srcdir)/v850-dc
tmp-igen: $(IGEN_INSN) $(IGEN_DC) ../igen/igen
- ../igen/igen \
+ $(IGEN) \
$(IGEN_TRACE) \
-G gen-direct-access \
-G gen-zero-r0 \
--
2.30.1
next prev parent reply other threads:[~2021-04-07 14:19 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-05 14:58 Simon Marchi via Gdb-patches
2021-04-05 16:23 ` Mike Frysinger via Gdb-patches
2021-04-05 18:46 ` Simon Marchi via Gdb-patches
2021-04-05 21:51 ` Mike Frysinger via Gdb-patches
2021-04-06 1:36 ` Simon Marchi via Gdb-patches
2021-04-06 10:41 ` Mike Frysinger via Gdb-patches
2021-04-06 13:28 ` Simon Marchi via Gdb-patches
2021-04-06 13:45 ` Tom Tromey
2021-04-06 18:01 ` Philippe Waroquiers via Gdb-patches
2021-04-06 22:45 ` Mike Frysinger via Gdb-patches
2021-04-07 1:45 ` Simon Marchi via Gdb-patches
2021-04-07 11:38 ` Mike Frysinger via Gdb-patches
2021-04-07 14:19 ` Simon Marchi via Gdb-patches [this message]
2021-04-08 4:51 ` Mike Frysinger via Gdb-patches
2021-04-08 13:52 ` Simon Marchi via Gdb-patches
2021-04-08 4:50 ` Mike Frysinger via Gdb-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84f2de14-6806-2cbc-0657-c64fe6a5f936@polymtl.ca \
--to=gdb-patches@sourceware.org \
--cc=simon.marchi@polymtl.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox