Message ID | 14db3354-56a5-86cc-b840-010c9ba7fc83@gmail.com |
---|---|
State | New |
Headers | show |
Series | avoid -Waddress in vfprintf-internal.c | expand |
* Martin Sebor via Libc-alpha: > Building Glibc with a GCC 12 enhanced to detect more instances > of comparing addresses to null that are guaranteed to evaluate > to a constanst triggers a large number of such instancesl. > The warnings, all isolated to the same file, are valid and > intended but the Glibc code is safe. They show up because > the comparison is in a macro to which either null or a constant > address of an array element are alternately passed as an argument. > > The attached patch avoids these warnings by introducing local > variables for the address being compared (an array element) > as well as for the null pointer. > > Tested by building Glibc on x86_64, verifying the warnings > are gone, and by running the testsuite and checking for new > failures. I'm testing patches to clean this up, with a net reduction in the number of lines of code. Thanks, Florian
* Martin Sebor via Libc-alpha: > Building Glibc with a GCC 12 enhanced to detect more instances > of comparing addresses to null that are guaranteed to evaluate > to a constanst triggers a large number of such instancesl. > The warnings, all isolated to the same file, are valid and > intended but the Glibc code is safe. They show up because > the comparison is in a macro to which either null or a constant > address of an array element are alternately passed as an argument. > > The attached patch avoids these warnings by introducing local > variables for the address being compared (an array element) > as well as for the null pointer. > > Tested by building Glibc on x86_64, verifying the warnings > are gone, and by running the testsuite and checking for new > failures. I believe the issue addressed in this patch no longer exists in current Git. Would you please verify? Thanks, Florian
On 9/27/21 7:48 AM, Florian Weimer wrote: > * Martin Sebor via Libc-alpha: > >> Building Glibc with a GCC 12 enhanced to detect more instances >> of comparing addresses to null that are guaranteed to evaluate >> to a constanst triggers a large number of such instancesl. >> The warnings, all isolated to the same file, are valid and >> intended but the Glibc code is safe. They show up because >> the comparison is in a macro to which either null or a constant >> address of an array element are alternately passed as an argument. >> >> The attached patch avoids these warnings by introducing local >> variables for the address being compared (an array element) >> as well as for the null pointer. >> >> Tested by building Glibc on x86_64, verifying the warnings >> are gone, and by running the testsuite and checking for new >> failures. > > I believe the issue addressed in this patch no longer exists in current > Git. Would you please verify? I don't see the warnings in my latest build anymore. The GCC patch that enhances the warning is still waiting for formal approval but I don't expect to make any substantive changes to it. I've resolved the bug I raised to keep track of the warnings (BZ #28368). Thanks Martin
On 9/27/21 19:58, Martin Sebor via Libc-alpha wrote: > On 9/27/21 7:48 AM, Florian Weimer wrote: >> * Martin Sebor via Libc-alpha: >> >>> Building Glibc with a GCC 12 enhanced to detect more instances >>> of comparing addresses to null that are guaranteed to evaluate >>> to a constanst triggers a large number of such instancesl. >>> The warnings, all isolated to the same file, are valid and >>> intended but the Glibc code is safe. They show up because >>> the comparison is in a macro to which either null or a constant >>> address of an array element are alternately passed as an argument. >>> >>> The attached patch avoids these warnings by introducing local >>> variables for the address being compared (an array element) >>> as well as for the null pointer. >>> >>> Tested by building Glibc on x86_64, verifying the warnings >>> are gone, and by running the testsuite and checking for new >>> failures. >> >> I believe the issue addressed in this patch no longer exists in current >> Git. Would you please verify? > > I don't see the warnings in my latest build anymore. The GCC patch > that enhances the warning is still waiting for formal approval but > I don't expect to make any substantive changes to it. I've resolved > the bug I raised to keep track of the warnings (BZ #28368). Is this patch still required for any other reasons?
On 10/19/21 10:26 AM, Carlos O'Donell wrote: > On 9/27/21 19:58, Martin Sebor via Libc-alpha wrote: >> On 9/27/21 7:48 AM, Florian Weimer wrote: >>> * Martin Sebor via Libc-alpha: >>> >>>> Building Glibc with a GCC 12 enhanced to detect more instances >>>> of comparing addresses to null that are guaranteed to evaluate >>>> to a constanst triggers a large number of such instancesl. >>>> The warnings, all isolated to the same file, are valid and >>>> intended but the Glibc code is safe. They show up because >>>> the comparison is in a macro to which either null or a constant >>>> address of an array element are alternately passed as an argument. >>>> >>>> The attached patch avoids these warnings by introducing local >>>> variables for the address being compared (an array element) >>>> as well as for the null pointer. >>>> >>>> Tested by building Glibc on x86_64, verifying the warnings >>>> are gone, and by running the testsuite and checking for new >>>> failures. >>> >>> I believe the issue addressed in this patch no longer exists in current >>> Git. Would you please verify? >> >> I don't see the warnings in my latest build anymore. The GCC patch >> that enhances the warning is still waiting for formal approval but >> I don't expect to make any substantive changes to it. I've resolved >> the bug I raised to keep track of the warnings (BZ #28368). > > Is this patch still required for any other reasons? I don't think so. Thanks Martin
diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c index 3f3d1e148a..1a9f94e930 100644 --- a/stdio-common/vfprintf-internal.c +++ b/stdio-common/vfprintf-internal.c @@ -1447,6 +1447,9 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags) UCHAR_T pad = L_(' ');/* Padding character. */ CHAR_T spec; + /* Passed to the process_arg macro instead of NULL to avoid -Waddress. */ + struct printf_spec * null_pspec = NULL; + workend = work_buffer + WORK_BUFFER_SIZE; /* Get current character in format string. */ @@ -1643,8 +1646,8 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags) /* Process current format. */ while (1) { - process_arg (((struct printf_spec *) NULL)); - process_string_arg (((struct printf_spec *) NULL)); + process_arg (null_pspec); + process_string_arg (null_pspec); LABEL (form_unknown): if (spec == L_('\0')) @@ -1904,53 +1907,55 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format, union printf_arg the_arg; CHAR_T *string; /* Pointer to argument string. */ + struct printf_spec * const pspec = &specs[nspecs_done]; + /* Fill variables from values in struct. */ - int alt = specs[nspecs_done].info.alt; - int space = specs[nspecs_done].info.space; - int left = specs[nspecs_done].info.left; - int showsign = specs[nspecs_done].info.showsign; - int group = specs[nspecs_done].info.group; - int is_long_double = specs[nspecs_done].info.is_long_double; - int is_short = specs[nspecs_done].info.is_short; - int is_char = specs[nspecs_done].info.is_char; - int is_long = specs[nspecs_done].info.is_long; - int width = specs[nspecs_done].info.width; - int prec = specs[nspecs_done].info.prec; - int use_outdigits = specs[nspecs_done].info.i18n; - char pad = specs[nspecs_done].info.pad; - CHAR_T spec = specs[nspecs_done].info.spec; + int alt = pspec->info.alt; + int space = pspec->info.space; + int left = pspec->info.left; + int showsign = pspec->info.showsign; + int group = pspec->info.group; + int is_long_double = pspec->info.is_long_double; + int is_short = pspec->info.is_short; + int is_char = pspec->info.is_char; + int is_long = pspec->info.is_long; + int width = pspec->info.width; + int prec = pspec->info.prec; + int use_outdigits = pspec->info.i18n; + char pad = pspec->info.pad; + CHAR_T spec = pspec->info.spec; CHAR_T *workend = work_buffer + WORK_BUFFER_SIZE; /* Fill in last information. */ - if (specs[nspecs_done].width_arg != -1) + if (pspec->width_arg != -1) { /* Extract the field width from an argument. */ - specs[nspecs_done].info.width = - args_value[specs[nspecs_done].width_arg].pa_int; + pspec->info.width = + args_value[pspec->width_arg].pa_int; - if (specs[nspecs_done].info.width < 0) + if (pspec->info.width < 0) /* If the width value is negative left justification is selected and the value is taken as being positive. */ { - specs[nspecs_done].info.width *= -1; - left = specs[nspecs_done].info.left = 1; + pspec->info.width *= -1; + left = pspec->info.left = 1; } - width = specs[nspecs_done].info.width; + width = pspec->info.width; } - if (specs[nspecs_done].prec_arg != -1) + if (pspec->prec_arg != -1) { /* Extract the precision from an argument. */ - specs[nspecs_done].info.prec = - args_value[specs[nspecs_done].prec_arg].pa_int; + pspec->info.prec = + args_value[pspec->prec_arg].pa_int; - if (specs[nspecs_done].info.prec < 0) + if (pspec->info.prec < 0) /* If the precision is negative the precision is omitted. */ - specs[nspecs_done].info.prec = -1; + pspec->info.prec = -1; - prec = specs[nspecs_done].info.prec; + prec = pspec->info.prec; } /* Process format specifiers. */ @@ -1963,17 +1968,17 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format, && __printf_function_table != NULL && __printf_function_table[(size_t) spec] != NULL) { - const void **ptr = alloca (specs[nspecs_done].ndata_args + const void **ptr = alloca (pspec->ndata_args * sizeof (const void *)); /* Fill in an array of pointers to the argument values. */ - for (unsigned int i = 0; i < specs[nspecs_done].ndata_args; + for (unsigned int i = 0; i < pspec->ndata_args; ++i) - ptr[i] = &args_value[specs[nspecs_done].data_arg + i]; + ptr[i] = &args_value[pspec->data_arg + i]; /* Call the function. */ function_done = __printf_function_table[(size_t) spec] - (s, &specs[nspecs_done].info, ptr); + (s, &pspec->info, ptr); if (function_done != -2) { @@ -1993,23 +1998,23 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format, JUMP (spec, step4_jumps); - process_arg ((&specs[nspecs_done])); - process_string_arg ((&specs[nspecs_done])); + process_arg (pspec); + process_string_arg (pspec); LABEL (form_unknown): { unsigned int i; const void **ptr; - ptr = alloca (specs[nspecs_done].ndata_args + ptr = alloca (pspec->ndata_args * sizeof (const void *)); /* Fill in an array of pointers to the argument values. */ - for (i = 0; i < specs[nspecs_done].ndata_args; ++i) - ptr[i] = &args_value[specs[nspecs_done].data_arg + i]; + for (i = 0; i < pspec->ndata_args; ++i) + ptr[i] = &args_value[pspec->data_arg + i]; /* Call the function. */ - function_done = printf_unknown (s, &specs[nspecs_done].info, + function_done = printf_unknown (s, &pspec->info, ptr); /* If an error occurred we don't have information about # @@ -2027,9 +2032,7 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format, } /* Write the following constant string. */ - outstring (specs[nspecs_done].end_of_fmt, - specs[nspecs_done].next_fmt - - specs[nspecs_done].end_of_fmt); + outstring (pspec->end_of_fmt, pspec->next_fmt - pspec->end_of_fmt); } all_done: scratch_buffer_free (&argsbuf);