From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender-0.a4lg.com [IPv6:2401:2500:203:30b:4000:6bfe:4757:0]) by sourceware.org (Postfix) with ESMTPS id DE19C384D14B for ; Thu, 6 Oct 2022 05:39:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DE19C384D14B Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail-sender-0.a4lg.com (Postfix) with ESMTPSA id 4E20B300089; Thu, 6 Oct 2022 05:39:08 +0000 (UTC) Message-ID: <22f70b64-0bf9-96f8-a734-d16bbec6504a@irq.a4lg.com> Date: Thu, 6 Oct 2022 14:39:07 +0900 Mime-Version: 1.0 Subject: Re: [PATCH 4/4] sim: Suppress non-literal printf warning Content-Language: en-US To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <35eaee9855aa3882a1ecc9de9d62c0f8b2fe0e7c.1663073826.git.research_trasio@irq.a4lg.com> <87h70ifq8c.fsf@redhat.com> From: Tsukasa OI In-Reply-To: <87h70ifq8c.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Oct 2022 05:39:12 -0000 On 2022/10/05 20:45, Andrew Burgess wrote: > Tsukasa OI writes: > >> Clang generates a warning if the format parameter of a printf-like function >> is not a literal. However, on hw_vabort, it's unavoidable to use non- >> literal as a format string (unless we make huge redesign). >> >> We have "include/diagnostics.h" to suppress certain warnings only when >> necessary. Because DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL can suppress >> warnings when the format parameter of a printf-like function is not a >> literal, this commit adds this (only where necessary) to suppress this >> error with "-Werror", the default configuration. >> >> sim/ChangeLog: >> >> * common/sim-hw.c (hw_vabort): Suppress non-literal printf warning >> by using DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL. >> --- >> sim/common/sim-hw.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c >> index cece5638bc9..36f355d2262 100644 >> --- a/sim/common/sim-hw.c >> +++ b/sim/common/sim-hw.c >> @@ -425,10 +425,13 @@ hw_vabort (struct hw *me, >> strcat (msg, ": "); >> strcat (msg, fmt); >> /* report the problem */ >> + DIAGNOSTIC_PUSH >> + DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL >> sim_engine_vabort (hw_system (me), >> STATE_HW (hw_system (me))->cpu, >> STATE_HW (hw_system (me))->cia, >> msg, ap); >> + DIAGNOSTIC_POP > > Rather than disabling diagnostics, I'd like to propose the patch below > which expands FMT and AP within sim-hw.c, then passes the expanded > string through to sim_engine_abort. What do you think of this? Ah, It took a while to understand but makes sense to me. I just needed to add ATTRIBUTE_PRINTF (2, 0) to suppress "-Werror -Wformat-nonliteral" but I prefer to use your patch instead. > > My motivation is to avoid disabling diagnostics as much as possible. I support your opinion as possible. I sometimes disable some warnings intentionally but it's because I thought disabling the warning is the only viable solution. In this case, it wasn't. Thanks, Tsukasa > > As far as I can tell the host_callback_struct::evprintf_filtered > callback is just the standard printf API, so using vsnprintf should > expand everything correctly. > > Thanks, > Andrew > > --- > > diff --git a/sim/common/sim-hw.c b/sim/common/sim-hw.c > index cece5638bc9..7bfe91e4ae2 100644 > --- a/sim/common/sim-hw.c > +++ b/sim/common/sim-hw.c > @@ -408,8 +408,11 @@ hw_vabort (struct hw *me, > const char *fmt, > va_list ap) > { > + int len; > const char *name; > char *msg; > + va_list cpy; > + > /* find an identity */ > if (me != NULL && hw_path (me) != NULL && hw_path (me) [0] != '\0') > name = hw_path (me); > @@ -419,16 +422,19 @@ hw_vabort (struct hw *me, > name = hw_family (me); > else > name = "device"; > - /* construct an updated format string */ > - msg = alloca (strlen (name) + strlen (": ") + strlen (fmt) + 1); > - strcpy (msg, name); > - strcat (msg, ": "); > - strcat (msg, fmt); > + > + /* Expand FMT and AP into MSG buffer. */ > + va_copy (cpy, ap); > + len = vsnprintf (NULL, 0, fmt, cpy) + 1; > + va_end (cpy); > + msg = alloca (len); > + vsnprintf (msg, len, fmt, ap); > + > /* report the problem */ > - sim_engine_vabort (hw_system (me), > - STATE_HW (hw_system (me))->cpu, > - STATE_HW (hw_system (me))->cia, > - msg, ap); > + sim_engine_abort (hw_system (me), > + STATE_HW (hw_system (me))->cpu, > + STATE_HW (hw_system (me))->cia, > + "%s: %s", name, msg); > } > > void >