Message ID | 20230206222513.1773039-4-iii@linux.ibm.com |
---|---|
State | Superseded |
Headers |
Return-Path: <elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B9F1C385B515 for <patchwork@sourceware.org>; Mon, 6 Feb 2023 22:26:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B9F1C385B515 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1675722362; bh=9h1l7vYLA1tht5NpLnQE0ZQu1BsPN3/xQjVWUs9Pfcc=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Help:List-Subscribe:From: Reply-To:From; b=OSvL7NOmi/AkpcbNj5A4SOh3OU4qqL0nYowt8ZR9cntSxPL09t3sjxU26shvmtP6A FViZyqsAFq+R5aB8YzdPmcc1jiYABR1CYw/ZHk1h2EPfJe3B3403tzMxDLZJtJE9pK eoUyDDssCaLrQ2IjQXDzBTeCAcdgtK+sN7uDZXWE= X-Original-To: elfutils-devel@sourceware.org Delivered-To: elfutils-devel@sourceware.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id DB500385840C for <elfutils-devel@sourceware.org>; Mon, 6 Feb 2023 22:25:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DB500385840C Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 316LBUIb022111 for <elfutils-devel@sourceware.org>; Mon, 6 Feb 2023 22:25:31 GMT Received: from ppma03ams.nl.ibm.com (62.31.33a9.ip4.static.sl-reverse.com [169.51.49.98]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3nk98d1r04-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for <elfutils-devel@sourceware.org>; Mon, 06 Feb 2023 22:25:30 +0000 Received: from pps.filterd (ppma03ams.nl.ibm.com [127.0.0.1]) by ppma03ams.nl.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 316GRFQH002355 for <elfutils-devel@sourceware.org>; Mon, 6 Feb 2023 22:25:28 GMT Received: from smtprelay04.fra02v.mail.ibm.com ([9.218.2.228]) by ppma03ams.nl.ibm.com (PPS) with ESMTPS id 3nhf06jutm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for <elfutils-devel@sourceware.org>; Mon, 06 Feb 2023 22:25:28 +0000 Received: from smtpav06.fra02v.mail.ibm.com (smtpav06.fra02v.mail.ibm.com [10.20.54.105]) by smtprelay04.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 316MPORE24641878 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 6 Feb 2023 22:25:25 GMT Received: from smtpav06.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id CD61920075; Mon, 6 Feb 2023 22:25:24 +0000 (GMT) Received: from smtpav06.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8302120074; Mon, 6 Feb 2023 22:25:24 +0000 (GMT) Received: from heavy.lan (unknown [9.179.9.231]) by smtpav06.fra02v.mail.ibm.com (Postfix) with ESMTP; Mon, 6 Feb 2023 22:25:24 +0000 (GMT) To: elfutils-devel@sourceware.org Cc: Ilya Leoshkevich <iii@linux.ibm.com> Subject: [PATCH RFC 03/11] printversion: Fix unused variable Date: Mon, 6 Feb 2023 23:25:05 +0100 Message-Id: <20230206222513.1773039-4-iii@linux.ibm.com> X-Mailer: git-send-email 2.39.1 In-Reply-To: <20230206222513.1773039-1-iii@linux.ibm.com> References: <20230206222513.1773039-1-iii@linux.ibm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: QJf-hcO3dQINehVT-4X0rEfXMhSQkbCx X-Proofpoint-ORIG-GUID: QJf-hcO3dQINehVT-4X0rEfXMhSQkbCx X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1 definitions=2023-02-06_07,2023-02-06_03,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 mlxlogscore=974 impostorscore=0 mlxscore=0 priorityscore=1501 phishscore=0 adultscore=0 suspectscore=0 clxscore=1015 spamscore=0 lowpriorityscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2302060191 X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, 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: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list <elfutils-devel.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/elfutils-devel/> List-Help: <mailto:elfutils-devel-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=subscribe> From: Ilya Leoshkevich via Elfutils-devel <elfutils-devel@sourceware.org> Reply-To: Ilya Leoshkevich <iii@linux.ibm.com> Errors-To: elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org Sender: "Elfutils-devel" <elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
Add Memory Sanitizer support
|
|
Commit Message
Ilya Leoshkevich
Feb. 6, 2023, 10:25 p.m. UTC
clang complains:
debuginfod.cxx:354:1: error: unused variable 'apba__' [-Werror,-Wunused-const-variable]
ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
^
../lib/printversion.h:47:21: note: expanded from macro 'ARGP_PROGRAM_BUG_ADDRESS_DEF'
const char *const apba__ __asm ("argp_program_bug_address")
^
This is as expected: it's used by argp via the
"argp_program_bug_address" name, which is not visible on the C level.
Add __attribute__ ((used)) to make sure that the compiler emits it.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
lib/printversion.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Comments
Hi Ilya (CC Frank), On Mon, Feb 06, 2023 at 11:25:05PM +0100, Ilya Leoshkevich via Elfutils-devel wrote: > clang complains: > > debuginfod.cxx:354:1: error: unused variable 'apba__' [-Werror,-Wunused-const-variable] > ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT; > ^ > ../lib/printversion.h:47:21: note: expanded from macro 'ARGP_PROGRAM_BUG_ADDRESS_DEF' > const char *const apba__ __asm ("argp_program_bug_address") > ^ > > This is as expected: it's used by argp via the > "argp_program_bug_address" name, which is not visible on the C level. > Add __attribute__ ((used)) to make sure that the compiler emits it. Actually I think it found a real issue. Note that the same construct is used the C eu tools. But in debuginfod.cxx it says: /* Name and version of program. */ /* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++ /* Bug report address. */ ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT; Note how ARGP_PROGRAM_VERSION_HOOK_DEF is commented out and in main it has: /* Parse and process arguments. */ int remaining; argp_program_version_hook = print_version; // this works (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL); So it sets print_version, but not argp_program_bug_address. And indeed debuginfod --help is missing the bug reporting address. I don't really know/understand why the printversion.h macro trick doesn't work with C++ (symbol mangling?). But we do need at least this patch: diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 4271acf4..0ec326d5 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -4172,6 +4165,7 @@ main (int argc, char *argv[]) /* Parse and process arguments. */ int remaining; argp_program_version_hook = print_version; // this works + argp_program_bug_address = PACKAGE_BUGREPORT; (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL); if (remaining != argc) error (EXIT_FAILURE, 0, Then debuginfod --help will say: Report bugs to https://sourceware.org/bugzilla. That of course doesn't help with the -Wunused-const-variable warning. If we cannot figure out the magic variable naming trick with with C++ then maybe we can just not include printversion.h and do it "by hand"? (as attached) Cheers, Mark diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 4271acf4..1ea90645 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -45,7 +45,6 @@ extern "C" { #endif extern "C" { -#include "printversion.h" #include "system.h" } @@ -345,13 +344,11 @@ static const char DEBUGINFOD_SQLITE_CLEANUP_DDL[] = ; - - -/* Name and version of program. */ -/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++ - -/* Bug report address. */ -ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT; +extern "C" { +/* Defined in version.c. Explicitly declared here because including + print_version.h magic variable tricks don't work in C++. */ +void print_version (FILE *stream, struct argp_state *state); +} /* Definitions of arguments for argp functions. */ static const struct argp_option options[] = @@ -4172,6 +4169,7 @@ main (int argc, char *argv[]) /* Parse and process arguments. */ int remaining; argp_program_version_hook = print_version; // this works + argp_program_bug_address = PACKAGE_BUGREPORT; (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL); if (remaining != argc) error (EXIT_FAILURE, 0,
On Tue, 2023-02-07 at 21:44 +0100, Mark Wielaard wrote: > Hi Ilya (CC Frank), > > On Mon, Feb 06, 2023 at 11:25:05PM +0100, Ilya Leoshkevich via > Elfutils-devel wrote: > > clang complains: > > > > debuginfod.cxx:354:1: error: unused variable 'apba__' [- > > Werror,-Wunused-const-variable] > > ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT; > > ^ > > ../lib/printversion.h:47:21: note: expanded from macro > > 'ARGP_PROGRAM_BUG_ADDRESS_DEF' > > const char *const apba__ __asm ("argp_program_bug_address") > > ^ > > > > This is as expected: it's used by argp via the > > "argp_program_bug_address" name, which is not visible on the C > > level. > > Add __attribute__ ((used)) to make sure that the compiler emits it. > > Actually I think it found a real issue. Note that the same construct > is used the C eu tools. But in debuginfod.cxx it says: > > /* Name and version of program. */ > /* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this > simple for C++ > > /* Bug report address. */ > ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT; > > Note how ARGP_PROGRAM_VERSION_HOOK_DEF is commented out and in main > it > has: > > /* Parse and process arguments. */ > int remaining; > argp_program_version_hook = print_version; // this works > (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, > NULL); > > So it sets print_version, but not argp_program_bug_address. > And indeed debuginfod --help is missing the bug reporting address. > > I don't really know/understand why the printversion.h macro trick > doesn't work with C++ (symbol mangling?). But we do need at least > this patch: > > diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx > index 4271acf4..0ec326d5 100644 > --- a/debuginfod/debuginfod.cxx > +++ b/debuginfod/debuginfod.cxx > @@ -4172,6 +4165,7 @@ main (int argc, char *argv[]) > /* Parse and process arguments. */ > int remaining; > argp_program_version_hook = print_version; // this works > + argp_program_bug_address = PACKAGE_BUGREPORT; > (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, > NULL); > if (remaining != argc) > error (EXIT_FAILURE, 0, > > Then debuginfod --help will say: Report bugs to > https://sourceware.org/bugzilla. > > That of course doesn't help with the -Wunused-const-variable warning. > > If we cannot figure out the magic variable naming trick with with C++ > then maybe we can just not include printversion.h and do it "by > hand"? > (as attached) > > Cheers, > > Mark If I build: const char *const apba__ __asm ("argp_program_bug_address") \ __attribute__ ((used)) = "foobarbaz"; with C and C++, the difference is going to be: @@ -1,6 +1,5 @@ .file "1.c" .text - .globl argp_program_bug_address .section .rodata.str1.1,"aMS",@progbits,1 .LC0: .string "foobarbaz" This must have to do with C and C++ standards treating const differently [1]. The solution is to add extern: --- a/lib/printversion.h +++ b/lib/printversion.h @@ -44,6 +44,7 @@ void print_version (FILE *stream, struct argp_state *state); void (*const apvh) (FILE *, struct argp_state *) \ __asm ("argp_program_version_hook") #define ARGP_PROGRAM_BUG_ADDRESS_DEF \ + extern const char *const apba__; \ const char *const apba__ __asm ("argp_program_bug_address") \ __attribute__ ((used)) I can include this in v2 if it works for you. [1] https://stackoverflow.com/questions/8908071/const-correctness-in-c-vs-c
Hi Ilya, On Wed, 2023-02-08 at 13:22 +0100, Ilya Leoshkevich wrote: > If I build: > > const char *const apba__ __asm ("argp_program_bug_address") \ > __attribute__ ((used)) = "foobarbaz"; > > with C and C++, the difference is going to be: > > @@ -1,6 +1,5 @@ > .file "1.c" > .text > - .globl argp_program_bug_address > .section .rodata.str1.1,"aMS",@progbits,1 > .LC0: > .string "foobarbaz" > > This must have to do with C and C++ standards treating const > differently [1]. The solution is to add extern: > > --- a/lib/printversion.h > +++ b/lib/printversion.h > @@ -44,6 +44,7 @@ void print_version (FILE *stream, struct argp_state > *state); > void (*const apvh) (FILE *, struct argp_state *) \ > __asm ("argp_program_version_hook") > #define ARGP_PROGRAM_BUG_ADDRESS_DEF \ > + extern const char *const apba__; \ > const char *const apba__ __asm ("argp_program_bug_address") \ > __attribute__ ((used)) > > I can include this in v2 if it works for you. > > [1] > https://stackoverflow.com/questions/8908071/const-correctness-in-c-vs-c O nice, that explains it. But then in that case I don't think you need the __attribute__ ((used)) anymore. Also as a nitpick the multiline define could be just a single line if you declare the extern on its own in printversion.h. And it would be nice to also cleanup apvh/argp_program_version_hook so it too works with c++, so we can remove the hack in debuginfod.cxx. Does the following work for you? diff --git a/lib/printversion.h b/lib/printversion.h index a9e059ff..bc9ca7ae 100644 --- a/lib/printversion.h +++ b/lib/printversion.h @@ -40,9 +40,11 @@ void print_version (FILE *stream, struct argp_state *state); variables as non-const (which is correct in general). But we can do better, it is not going to change. So we want to move them into the .rodata section. Define macros to do the trick. */ +extern void (*const apvh) (FILE *, struct argp_state *); #define ARGP_PROGRAM_VERSION_HOOK_DEF \ void (*const apvh) (FILE *, struct argp_state *) \ __asm ("argp_program_version_hook") +extern const char *const apba__; #define ARGP_PROGRAM_BUG_ADDRESS_DEF \ const char *const apba__ __asm ("argp_program_bug_address") diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 4271acf4..99b1f2b9 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -348,7 +348,7 @@ static const char DEBUGINFOD_SQLITE_CLEANUP_DDL[] = /* Name and version of program. */ -/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this simple for C++ +ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; /* Bug report address. */ ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT; @@ -4171,7 +4171,6 @@ main (int argc, char *argv[]) /* Parse and process arguments. */ int remaining; - argp_program_version_hook = print_version; // this works (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL); if (remaining != argc) error (EXIT_FAILURE, 0, Thanks, Mark
On Thu, 2023-02-09 at 15:04 +0100, Mark Wielaard wrote: > Hi Ilya, > > On Wed, 2023-02-08 at 13:22 +0100, Ilya Leoshkevich wrote: > > If I build: > > > > const char *const apba__ __asm ("argp_program_bug_address") \ > > __attribute__ ((used)) = "foobarbaz"; > > > > with C and C++, the difference is going to be: > > > > @@ -1,6 +1,5 @@ > > .file "1.c" > > .text > > - .globl argp_program_bug_address > > .section .rodata.str1.1,"aMS",@progbits,1 > > .LC0: > > .string "foobarbaz" > > > > This must have to do with C and C++ standards treating const > > differently [1]. The solution is to add extern: > > > > --- a/lib/printversion.h > > +++ b/lib/printversion.h > > @@ -44,6 +44,7 @@ void print_version (FILE *stream, struct > > argp_state > > *state); > > void (*const apvh) (FILE *, struct argp_state *) \ > > __asm ("argp_program_version_hook") > > #define ARGP_PROGRAM_BUG_ADDRESS_DEF \ > > + extern const char *const apba__; \ > > const char *const apba__ __asm ("argp_program_bug_address") \ > > __attribute__ ((used)) > > > > I can include this in v2 if it works for you. > > > > [1] > > https://stackoverflow.com/questions/8908071/const-correctness-in-c-vs-c > > O nice, that explains it. But then in that case I don't think you > need > the __attribute__ ((used)) anymore. > > Also as a nitpick the multiline define could be just a single line if > you declare the extern on its own in printversion.h. > > And it would be nice to also cleanup apvh/argp_program_version_hook > so > it too works with c++, so we can remove the hack in debuginfod.cxx. > > Does the following work for you? > > diff --git a/lib/printversion.h b/lib/printversion.h > index a9e059ff..bc9ca7ae 100644 > --- a/lib/printversion.h > +++ b/lib/printversion.h > @@ -40,9 +40,11 @@ void print_version (FILE *stream, struct > argp_state *state); > variables as non-const (which is correct in general). But we can > do better, it is not going to change. So we want to move them > into > the .rodata section. Define macros to do the trick. */ > +extern void (*const apvh) (FILE *, struct argp_state *); > #define ARGP_PROGRAM_VERSION_HOOK_DEF \ > void (*const apvh) (FILE *, struct argp_state *) \ > __asm ("argp_program_version_hook") > +extern const char *const apba__; > #define ARGP_PROGRAM_BUG_ADDRESS_DEF \ > const char *const apba__ __asm ("argp_program_bug_address") > > diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx > index 4271acf4..99b1f2b9 100644 > --- a/debuginfod/debuginfod.cxx > +++ b/debuginfod/debuginfod.cxx > @@ -348,7 +348,7 @@ static const char DEBUGINFOD_SQLITE_CLEANUP_DDL[] > = > > > /* Name and version of program. */ > -/* ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; */ // not this > simple for C++ > +ARGP_PROGRAM_VERSION_HOOK_DEF = print_version; > > /* Bug report address. */ > ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT; > @@ -4171,7 +4171,6 @@ main (int argc, char *argv[]) > > /* Parse and process arguments. */ > int remaining; > - argp_program_version_hook = print_version; // this works > (void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, > NULL); > if (remaining != argc) > error (EXIT_FAILURE, 0, > > Thanks, > > Mark This works for me, I will add this to v3. Thanks!
diff --git a/lib/printversion.h b/lib/printversion.h index a9e059ff..d1c3a987 100644 --- a/lib/printversion.h +++ b/lib/printversion.h @@ -44,6 +44,7 @@ void print_version (FILE *stream, struct argp_state *state); void (*const apvh) (FILE *, struct argp_state *) \ __asm ("argp_program_version_hook") #define ARGP_PROGRAM_BUG_ADDRESS_DEF \ - const char *const apba__ __asm ("argp_program_bug_address") + const char *const apba__ __asm ("argp_program_bug_address") \ + __attribute__ ((used)) #endif // PRINTVERSION_H