[RFC,03/11] printversion: Fix unused variable

  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(-)
  

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!