* [PATCH] restore ancient -Waddress for weak symbols [PR33925] @ 2021-10-04 18:42 Martin Sebor 2021-10-04 21:37 ` Jason Merrill 2021-10-04 22:39 ` Eric Gallager 0 siblings, 2 replies; 14+ messages in thread From: Martin Sebor @ 2021-10-04 18:42 UTC (permalink / raw) To: Joseph S. Myers, Jason Merrill; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1189 bytes --] While resolving the recent -Waddress enhancement request (PR PR102103) I came across a 2007 problem report about GCC 4 having stopped warning for using the address of inline functions in equality comparisons with null. With inline functions being commonplace in C++ this seems like an important use case for the warning. The change that resulted in suppressing the warning in these cases was introduced inadvertently in a fix for PR 22252. To restore the warning, the attached patch enhances the decl_with_nonnull_addr_p() function to return true also for weak symbols for which a definition has been provided. Tested on x86_64-linux and by comparing the GCC output for new test cases to Clang's which diagnoses all but one instance of these cases with either -Wtautological-pointer-compare or -Wpointer-bool-conversion, depending on context. The one case where Clang doesn't issue a warning but GCC with the patch does is for a symbol explicitly declared with attribute weak for which a definition has been provided. I believe the address of such symbols is necessarily nonnull and so issuing the warning is helpful (both GCC and Clang fold such comparisons to a constant). Martin [-- Attachment #2: gcc-33925.diff --] [-- Type: text/x-patch, Size: 8025 bytes --] PR c/33925 - missing -Waddress with the address of an inline function gcc/c-family/ChangeLog: PR c/33925 * c-common.c (decl_with_nonnull_addr_p): Also return true for weak symbols for which a definition has been provided. gcc/testsuite/ChangeLog: PR c/33925 * c-c++-common/Waddress-5.c: New test. * g++.dg/warn/Waddress-7.C: New test. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 9d19e352725..ba02d3981c0 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -3395,7 +3395,8 @@ c_wrap_maybe_const (tree expr, bool non_const) /* Return whether EXPR is a declaration whose address can never be NULL. The address of the first struct member could be NULL only if it were - accessed through a NULL pointer, and such an access would be invalid. */ + accessed through a NULL pointer, and such an access would be invalid. + The address of a weak symbol may be null unless it has a definition. */ bool decl_with_nonnull_addr_p (const_tree expr) @@ -3404,7 +3405,9 @@ decl_with_nonnull_addr_p (const_tree expr) && (TREE_CODE (expr) == FIELD_DECL || TREE_CODE (expr) == PARM_DECL || TREE_CODE (expr) == LABEL_DECL - || !DECL_WEAK (expr))); + || !DECL_WEAK (expr) + || (DECL_INITIAL (expr) + && DECL_INITIAL (expr) != error_mark_node))); } /* Prepare expr to be an argument of a TRUTH_NOT_EXPR, diff --git a/gcc/testsuite/c-c++-common/Waddress-5.c b/gcc/testsuite/c-c++-common/Waddress-5.c new file mode 100644 index 00000000000..fa6658fa133 --- /dev/null +++ b/gcc/testsuite/c-c++-common/Waddress-5.c @@ -0,0 +1,133 @@ +/* PR c/33925 - missing -Waddress with the address of an inline function + { dg-do compile } + { dg-options "-Wall" } + { dg-require-weak "" } */ + +extern inline int eifn (void); +extern inline int eifn_def (void) { return 0; } + +static inline int sifn (void); +static inline int sifn_def (void) { return 0; } + +inline int ifn (void); +inline int ifn_def (void) { return 0; } + +extern __attribute__ ((weak)) int ewfn (void); +extern __attribute__ ((weak)) int ewfn_def (void) { return 0; } + +__attribute__ ((weak)) int wfn (void); +__attribute__ ((weak)) int wfn_def (void) { return 0; } + +static __attribute__((weakref ("ewfn"))) int swrfn; + +void test_function_eqz (int *p) +{ + *p++ = eifn == 0; // { dg-warning "-Waddress" } + *p++ = eifn_def == 0; // { dg-warning "-Waddress" } + *p++ = sifn == 0; // { dg-warning "-Waddress" } + *p++ = sifn_def == 0; // { dg-warning "-Waddress" } + *p++ = ifn == 0; // { dg-warning "-Waddress" } + *p++ = ifn_def == 0; // { dg-warning "-Waddress" } + *p++ = ewfn == 0; + *p++ = ewfn_def == 0; // { dg-warning "-Waddress" } + *p++ = wfn == 0; + *p++ = wfn_def == 0; // { dg-warning "-Waddress" } + *p++ = swrfn == 0; +} + + +int test_function_if (int i) +{ + if (eifn) // { dg-warning "-Waddress" } + i++; + if (eifn_def) // { dg-warning "-Waddress" } + i++; + if (sifn) // { dg-warning "-Waddress" } + i++; + if (sifn_def) // { dg-warning "-Waddress" } + i++; + if (ifn) // { dg-warning "-Waddress" } + i++; + if (ifn_def) // { dg-warning "-Waddress" } + i++; + if (ewfn) + i++; + if (ewfn_def) // { dg-warning "-Waddress" } + i++; + if (wfn) + i++; + if(wfn_def) // { dg-warning "-Waddress" } + i++; + if (swrfn) + i++; + return i; +} + + +extern int ei; +extern int ei_def = 1; + +static int si; +static int si_def = 1; + +int i; +int i_def = 1; + +extern __attribute__ ((weak)) int ewi; +extern __attribute__ ((weak)) int ewi_def = 1; + +__attribute__ ((weak)) int wi; +__attribute__ ((weak)) int wi_def = 1; + +static __attribute__((weakref ("ewi"))) int swri; + +void test_scalar (int *p) +{ + *p++ = &ei == 0; // { dg-warning "-Waddress" } + *p++ = &ei_def == 0; // { dg-warning "-Waddress" } + *p++ = &si == 0; // { dg-warning "-Waddress" } + *p++ = &si_def == 0; // { dg-warning "-Waddress" } + *p++ = &i == 0; // { dg-warning "-Waddress" } + *p++ = &i_def == 0; // { dg-warning "-Waddress" } + *p++ = &ewi == 0; + *p++ = &ewi_def == 0; // { dg-warning "-Waddress" } + *p++ = &wi == 0; + *p++ = &wi_def == 0; // { dg-warning "-Waddress" } + *p++ = &swri == 0; +} + + +extern int eia[]; +extern int eia_def[] = { 1 }; + +static int sia[1]; +static int sia_def[1] = { 1 }; + +int ia[1]; +int ia_def[] = { 1 }; + +extern __attribute__ ((weak)) int ewia[]; +extern __attribute__ ((weak)) int ewia_def[] = { 1 }; + +__attribute__ ((weak)) int wia[1]; +__attribute__ ((weak)) int wia_def[] = { 1 }; + +static __attribute__((weakref ("ewia"))) int swria[1]; + +void test_array (int *p) +{ + *p++ = eia == 0; // { dg-warning "-Waddress" } + *p++ = eia_def == 0; // { dg-warning "-Waddress" } + *p++ = sia == 0; // { dg-warning "-Waddress" } + *p++ = sia_def == 0; // { dg-warning "-Waddress" } + *p++ = ia == 0; // { dg-warning "-Waddress" } + *p++ = ia_def == 0; // { dg-warning "-Waddress" } + *p++ = ewia == 0; + *p++ = ewia_def == 0; // { dg-warning "-Waddress" } + *p++ = wia == 0; + *p++ = wia_def == 0; // { dg-warning "-Waddress" } + *p++ = swria == 0; +} + +/* { dg-prune-output "never defined" } + { dg-prune-output "initialized and declared 'extern'" } */ diff --git a/gcc/testsuite/g++.dg/warn/Waddress-7.C b/gcc/testsuite/g++.dg/warn/Waddress-7.C new file mode 100644 index 00000000000..efdafa50cd1 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Waddress-7.C @@ -0,0 +1,76 @@ +/* PR c/33925 - missing -Waddress with the address of an inline function + { dg-do compile } + { dg-options "-Wall" } */ + +struct A +{ + int mf (); + int mf_def () { return 0; } + + static int smf (); + static int smf_def () { return 0; } + + int mi; + static int smi; + static int smi_def; + + __attribute__ ((weak)) static int wsmi; + __attribute__ ((weak)) static int wsmi_def; + + int mia[]; + static int smia[]; + static int smia_def[]; + + __attribute__ ((weak)) static int wsmia[]; + __attribute__ ((weak)) static int wsmia_def[]; + + void test_waddress (bool*); +}; + + +/* __attribute__ ((weak)) static */ int A::smi_def = 0; +/* __attribute__ ((weak)) static */ int A::smia_def[] = { 0 }; + +/* __attribute__ ((weak)) static */ int A::wsmi_def = 0; +/* __attribute__ ((weak)) static */ int A::wsmia_def[] = { 0 }; + + + +void A::test_waddress (bool *p) +{ + if (mf) // { dg-error "cannot convert" } + p++; + if (mf_def) // { dg-error "cannot convert" } + p++; + + if (smf) // { dg-warning "-Waddress" } + p++; + if (smf_def) // { dg-warning "-Waddress" } + p++; + + if (&mi) // { dg-warning "-Waddress" } + p++; + if (&smi) // { dg-warning "-Waddress" } + p++; + if (&smi_def) // { dg-warning "-Waddress" } + p++; + + if (&wsmi) + p++; + + if (&wsmi_def) // { dg-warning "-Waddress" } + p++; + + if (mia) // { dg-warning "-Waddress" } + p++; + if (smia) // { dg-warning "-Waddress" } + p++; + if (smia_def) // { dg-warning "-Waddress" } + p++; + + if (wsmia) + p++; + + if (wsmia_def) // { dg-warning "-Waddress" } + p++; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-10-04 18:42 [PATCH] restore ancient -Waddress for weak symbols [PR33925] Martin Sebor @ 2021-10-04 21:37 ` Jason Merrill 2021-10-23 23:06 ` Martin Sebor 2021-10-04 22:39 ` Eric Gallager 1 sibling, 1 reply; 14+ messages in thread From: Jason Merrill @ 2021-10-04 21:37 UTC (permalink / raw) To: Martin Sebor, Joseph S. Myers; +Cc: gcc-patches On 10/4/21 14:42, Martin Sebor wrote: > While resolving the recent -Waddress enhancement request (PR > PR102103) I came across a 2007 problem report about GCC 4 having > stopped warning for using the address of inline functions in > equality comparisons with null. With inline functions being > commonplace in C++ this seems like an important use case for > the warning. > > The change that resulted in suppressing the warning in these > cases was introduced inadvertently in a fix for PR 22252. > > To restore the warning, the attached patch enhances > the decl_with_nonnull_addr_p() function to return true also for > weak symbols for which a definition has been provided. I think you probably want to merge this function with fold-const.c:maybe_nonzero_address, which already handles more cases. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-10-04 21:37 ` Jason Merrill @ 2021-10-23 23:06 ` Martin Sebor 2021-11-07 23:31 ` PING " Martin Sebor 2021-11-16 20:23 ` Jason Merrill 0 siblings, 2 replies; 14+ messages in thread From: Martin Sebor @ 2021-10-23 23:06 UTC (permalink / raw) To: Jason Merrill, Joseph S. Myers; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1689 bytes --] On 10/4/21 3:37 PM, Jason Merrill wrote: > On 10/4/21 14:42, Martin Sebor wrote: >> While resolving the recent -Waddress enhancement request (PR >> PR102103) I came across a 2007 problem report about GCC 4 having >> stopped warning for using the address of inline functions in >> equality comparisons with null. With inline functions being >> commonplace in C++ this seems like an important use case for >> the warning. >> >> The change that resulted in suppressing the warning in these >> cases was introduced inadvertently in a fix for PR 22252. >> >> To restore the warning, the attached patch enhances >> the decl_with_nonnull_addr_p() function to return true also for >> weak symbols for which a definition has been provided. > > I think you probably want to merge this function with > fold-const.c:maybe_nonzero_address, which already handles more cases. maybe_nonzero_address() doesn't behave quite like decl_with_nonnull_addr_p() expects and I'm reluctant to muck around with the former too much since it's used for codegen, while the latter just for warnings. (There is even a case where the functions don't behave the same, and would result in different warnings between C and C++ without some extra help.) So in the attached revision I just have maybe_nonzero_address() call decl_with_nonnull_addr_p() and then refine the failing (or uncertain) cases separately, with some overlap between them. Since I worked on this someone complained that some instances of the warning newly enhanced under PR102103 aren't suppresed in code resulting from macro expansion. Since it's trivial, I include the fix for that report in this patch as well. Tested on x86_64-linux. Martin [-- Attachment #2: gcc-33925.diff --] [-- Type: text/x-patch, Size: 15230 bytes --] Restore ancient -Waddress for weak symbols [PR33925]. Resolves: PR c/33925 - gcc -Waddress lost some useful warnings PR c/102867 - -Waddress from macro expansion in readelf.c gcc/c-family/ChangeLog: PR c++/33925 PR c/102867 * c-common.c (decl_with_nonnull_addr_p): Call maybe_nonzero_address and improve handling tof defined symbols. gcc/c/ChangeLog: PR c++/33925 PR c/102867 * c-typeck.c (maybe_warn_for_null_address): Suppress warnings for code resulting from macro expansion. gcc/cp/ChangeLog: PR c++/33925 PR c/102867 * typeck.c (warn_for_null_address): Suppress warnings for code resulting from macro expansion. gcc/ChangeLog: PR c++/33925 PR c/102867 * doc/invoke.texi (-Waddress): Update. * fold-const.c (maybe_nonzero_address): Declare extern. Simplify for readability. * tree.h (maybe_nonzero_address): Declare. gcc/testsuite/ChangeLog: PR c++/33925 PR c/102867 * g++.dg/warn/Walways-true-2.C: Adjust to avoid a valid warning. * c-c++-common/Waddress-5.c: New test. * c-c++-common/Waddress-6.c: New test. * g++.dg/warn/Waddress-7.C: New test. * g++.dg/warn/Walways-true-2.C: Adjust to avoid a valid warning. * gcc.dg/weak/weak-3.c: Expect a warning. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 32c7e3e8972..71cc74ac63d 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -3395,16 +3395,46 @@ c_wrap_maybe_const (tree expr, bool non_const) /* Return whether EXPR is a declaration whose address can never be NULL. The address of the first struct member could be NULL only if it were - accessed through a NULL pointer, and such an access would be invalid. */ + accessed through a NULL pointer, and such an access would be invalid. + The address of a weak symbol may be null unless it has a definition. */ bool decl_with_nonnull_addr_p (const_tree expr) { - return (DECL_P (expr) - && (TREE_CODE (expr) == FIELD_DECL - || TREE_CODE (expr) == PARM_DECL - || TREE_CODE (expr) == LABEL_DECL - || !DECL_WEAK (expr))); + if (maybe_nonzero_address (const_cast <tree>(expr)) > 0) + return true; + + if (!DECL_P (expr)) + return false; + + if (TREE_CODE (expr) == FIELD_DECL + || TREE_CODE (expr) == PARM_DECL + || TREE_CODE (expr) == LABEL_DECL) + return true; + + if (!VAR_OR_FUNCTION_DECL_P (expr)) + return false; + + if (!DECL_WEAK (expr)) + /* Ordinary (non-weak) symbols have nonnull addresses. */ + return true; + + if (DECL_INITIAL (expr) && DECL_INITIAL (expr) != error_mark_node) + /* Initialized weak symbols have nonnull addresses. */ + return true; + + if (DECL_EXTERNAL (expr) || !TREE_STATIC (expr)) + /* Uninitialized extern weak symbols and weak symbols with no + allocated stroage might have a null address. */ + return false; + + tree attribs = DECL_ATTRIBUTES (expr); + if (lookup_attribute ("weakref", attribs)) + /* Weakref symbols might have a null address unless their referent + is known not to. Don't bother following weakref targets here. */ + return false; + + return true; } /* Prepare expr to be an argument of a TRUTH_NOT_EXPR, diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 0aac978c02e..0ceedfa7b04 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -11571,7 +11571,10 @@ build_vec_cmp (tree_code code, tree type, static void maybe_warn_for_null_address (location_t loc, tree op, tree_code code) { - if (!warn_address || warning_suppressed_p (op, OPT_Waddress)) + /* Prevent warnings issued for macro expansion. */ + if (!warn_address + || warning_suppressed_p (op, OPT_Waddress) + || from_macro_expansion_at (loc)) return; if (TREE_CODE (op) == NOP_EXPR) diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index ab0f9da2552..ae40e27e1d5 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -4597,9 +4597,11 @@ build_vec_cmp (tree_code code, tree type, static void warn_for_null_address (location_t location, tree op, tsubst_flags_t complain) { + /* Prevent warnings issued for macro expansion. */ if (!warn_address || (complain & tf_warning) == 0 || c_inhibit_evaluation_warnings != 0 + || from_macro_expansion_at (location) || warning_suppressed_p (op, OPT_Waddress)) return; diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 71992b8c597..99dfce7201a 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -8603,6 +8603,8 @@ suppressed by casting the pointer operand to an integer type such as @code{inptr_t} or @code{uinptr_t}. Comparisons against string literals result in unspecified behavior and are not portable, and suggest the intent was to call @code{strcmp}. +The warning is suppressed if the suspicious expression is the result +of macro expansion. @option{-Waddress} warning is enabled by @option{-Wall}. @item -Wno-address-of-packed-member diff --git a/gcc/fold-const.c b/gcc/fold-const.c index ff23f12f33c..4a173d278fb 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -9900,18 +9900,20 @@ pointer_may_wrap_p (tree base, tree offset, poly_int64 bitpos) symbol), and a negative integer when the symbol is not yet in the symbol table and so whether or not its address is zero is unknown. For function local objects always return positive integer. */ -static int +int maybe_nonzero_address (tree decl) { - if (DECL_P (decl) && decl_in_symtab_p (decl)) + if (!DECL_P (decl)) + return -1; + + if (decl_in_symtab_p (decl)) if (struct symtab_node *symbol = symtab_node::get_create (decl)) return symbol->nonzero_address (); /* Function local objects are never NULL. */ - if (DECL_P (decl) - && (DECL_CONTEXT (decl) + if (DECL_CONTEXT (decl) && TREE_CODE (DECL_CONTEXT (decl)) == FUNCTION_DECL - && auto_var_in_fn_p (decl, DECL_CONTEXT (decl)))) + && auto_var_in_fn_p (decl, DECL_CONTEXT (decl))) return 1; return -1; diff --git a/gcc/testsuite/c-c++-common/Waddress-5.c b/gcc/testsuite/c-c++-common/Waddress-5.c new file mode 100644 index 00000000000..5d63c7dae7c --- /dev/null +++ b/gcc/testsuite/c-c++-common/Waddress-5.c @@ -0,0 +1,133 @@ +/* PR c/33925 - missing -Waddress with the address of an inline function + { dg-do compile } + { dg-options "-Wall" } + { dg-require-weak "" } */ + +extern inline int eifn (void); +extern inline int eifn_def (void) { return 0; } + +static inline int sifn (void); +static inline int sifn_def (void) { return 0; } + +inline int ifn (void); +inline int ifn_def (void) { return 0; } + +extern __attribute__ ((weak)) int ewfn (void); +extern __attribute__ ((weak)) int ewfn_def (void) { return 0; } + +__attribute__ ((weak)) int wfn (void); +__attribute__ ((weak)) int wfn_def (void) { return 0; } + +static __attribute__((weakref ("ewfn"))) int swrfn (void); + +void test_function_eqz (int *p) +{ + *p++ = eifn == 0; // { dg-warning "-Waddress" } + *p++ = eifn_def == 0; // { dg-warning "-Waddress" } + *p++ = sifn == 0; // { dg-warning "-Waddress" } + *p++ = sifn_def == 0; // { dg-warning "-Waddress" } + *p++ = ifn == 0; // { dg-warning "-Waddress" } + *p++ = ifn_def == 0; // { dg-warning "-Waddress" } + *p++ = ewfn == 0; + *p++ = ewfn_def == 0; // { dg-warning "-Waddress" } + *p++ = wfn == 0; + *p++ = wfn_def == 0; // { dg-warning "-Waddress" } + *p++ = swrfn == 0; +} + + +int test_function_if (int i) +{ + if (eifn) // { dg-warning "-Waddress" } + i++; + if (eifn_def) // { dg-warning "-Waddress" } + i++; + if (sifn) // { dg-warning "-Waddress" } + i++; + if (sifn_def) // { dg-warning "-Waddress" } + i++; + if (ifn) // { dg-warning "-Waddress" } + i++; + if (ifn_def) // { dg-warning "-Waddress" } + i++; + if (ewfn) + i++; + if (ewfn_def) // { dg-warning "-Waddress" } + i++; + if (wfn) + i++; + if(wfn_def) // { dg-warning "-Waddress" } + i++; + if (swrfn) + i++; + return i; +} + + +extern int ei; +extern int ei_def = 1; + +static int si; +static int si_def = 1; + +int i; +int i_def = 1; + +extern __attribute__ ((weak)) int ewi; // declaration (may be null) +extern __attribute__ ((weak)) int ewi_def = 1; + +__attribute__ ((weak)) int wi; // definition (cannot be bull) +__attribute__ ((weak)) int wi_def = 1; + +static __attribute__((weakref ("ewi"))) int swri; + +void test_scalar (int *p) +{ + *p++ = &ei == 0; // { dg-warning "-Waddress" } + *p++ = &ei_def == 0; // { dg-warning "-Waddress" } + *p++ = &si == 0; // { dg-warning "-Waddress" } + *p++ = &si_def == 0; // { dg-warning "-Waddress" } + *p++ = &i == 0; // { dg-warning "-Waddress" } + *p++ = &i_def == 0; // { dg-warning "-Waddress" } + *p++ = &ewi == 0; + *p++ = &ewi_def == 0; // { dg-warning "-Waddress" } + *p++ = &wi == 0; // { dg-warning "-Waddress" } + *p++ = &wi_def == 0; // { dg-warning "-Waddress" } + *p++ = &swri == 0; +} + + +extern int eia[]; +extern int eia_def[] = { 1 }; + +static int sia[1]; +static int sia_def[1] = { 1 }; + +int ia[1]; +int ia_def[] = { 1 }; + +extern __attribute__ ((weak)) int ewia[]; +extern __attribute__ ((weak)) int ewia_def[] = { 1 }; + +__attribute__ ((weak)) int wia[1]; // definition (cannot be null) +__attribute__ ((weak)) int wia_def[] = { 1 }; + +static __attribute__((weakref ("ewia"))) int swria[1]; + +void test_array (int *p) +{ + *p++ = eia == 0; // { dg-warning "-Waddress" } + *p++ = eia_def == 0; // { dg-warning "-Waddress" } + *p++ = sia == 0; // { dg-warning "-Waddress" } + *p++ = sia_def == 0; // { dg-warning "-Waddress" } + *p++ = ia == 0; // { dg-warning "-Waddress" } + *p++ = ia_def == 0; // { dg-warning "-Waddress" } + *p++ = ewia == 0; + *p++ = ewia_def == 0; // { dg-warning "-Waddress" } + *p++ = wia == 0; // { dg-warning "-Waddress" } + *p++ = wia_def == 0; // { dg-warning "-Waddress" } + *p++ = swria == 0; +} + +/* { dg-prune-output "never defined" } + { dg-prune-output "initialized and declared 'extern'" } */ diff --git a/gcc/testsuite/c-c++-common/Waddress-6.c b/gcc/testsuite/c-c++-common/Waddress-6.c new file mode 100644 index 00000000000..e66e6e4a0b0 --- /dev/null +++ b/gcc/testsuite/c-c++-common/Waddress-6.c @@ -0,0 +1,32 @@ +/* PR c/102867 - -Waddress from macro expansion in readelf.c + { dg-do compile } + { dg-options "-Wall" } */ + +#define F(x) ((&x) != 0) + +int warn_nomacro (int *p, int i) +{ + return &p[i] != 0; // { dg-warning "-Waddress" } +} + +int nowarn_macro_expansion (int *p, int i) +{ + // Verify that -Waddress isn't issued for code from macro expansion. + return F (p[i]); // { dg-bogus "-Waddress" } +} + +#define G(x, i) ((&x) + i) + +int warn_function_macro_expansion (int *p, int i) +{ + /* Verify that -Waddress is issued for code involving macro expansion + where the comparison takes place outside the macro. */ + return G (*p, i) != 0; // { dg-warning "-Waddress" } +} + +#define malloc __builtin_malloc + +int warn_object_macro_expansion (int *p, int i) +{ + return malloc != 0; // { dg-warning "-Waddress" } +} diff --git a/gcc/testsuite/g++.dg/warn/Waddress-7.C b/gcc/testsuite/g++.dg/warn/Waddress-7.C new file mode 100644 index 00000000000..efdafa50cd1 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Waddress-7.C @@ -0,0 +1,76 @@ +/* PR c/33925 - missing -Waddress with the address of an inline function + { dg-do compile } + { dg-options "-Wall" } */ + +struct A +{ + int mf (); + int mf_def () { return 0; } + + static int smf (); + static int smf_def () { return 0; } + + int mi; + static int smi; + static int smi_def; + + __attribute__ ((weak)) static int wsmi; + __attribute__ ((weak)) static int wsmi_def; + + int mia[]; + static int smia[]; + static int smia_def[]; + + __attribute__ ((weak)) static int wsmia[]; + __attribute__ ((weak)) static int wsmia_def[]; + + void test_waddress (bool*); +}; + + +/* __attribute__ ((weak)) static */ int A::smi_def = 0; +/* __attribute__ ((weak)) static */ int A::smia_def[] = { 0 }; + +/* __attribute__ ((weak)) static */ int A::wsmi_def = 0; +/* __attribute__ ((weak)) static */ int A::wsmia_def[] = { 0 }; + + + +void A::test_waddress (bool *p) +{ + if (mf) // { dg-error "cannot convert" } + p++; + if (mf_def) // { dg-error "cannot convert" } + p++; + + if (smf) // { dg-warning "-Waddress" } + p++; + if (smf_def) // { dg-warning "-Waddress" } + p++; + + if (&mi) // { dg-warning "-Waddress" } + p++; + if (&smi) // { dg-warning "-Waddress" } + p++; + if (&smi_def) // { dg-warning "-Waddress" } + p++; + + if (&wsmi) + p++; + + if (&wsmi_def) // { dg-warning "-Waddress" } + p++; + + if (mia) // { dg-warning "-Waddress" } + p++; + if (smia) // { dg-warning "-Waddress" } + p++; + if (smia_def) // { dg-warning "-Waddress" } + p++; + + if (wsmia) + p++; + + if (wsmia_def) // { dg-warning "-Waddress" } + p++; +} diff --git a/gcc/testsuite/g++.dg/warn/Walways-true-2.C b/gcc/testsuite/g++.dg/warn/Walways-true-2.C index 29a80e5c113..e951e95b336 100644 --- a/gcc/testsuite/g++.dg/warn/Walways-true-2.C +++ b/gcc/testsuite/g++.dg/warn/Walways-true-2.C @@ -9,7 +9,7 @@ extern int foo (int) __attribute__ ((weak)); -int i __attribute__ ((weak)); +extern int i __attribute__ ((weak)); void bar (int a) diff --git a/gcc/testsuite/gcc.dg/Walways-true-2.c b/gcc/testsuite/gcc.dg/Walways-true-2.c index 7f0bb7b10b9..ae3262b6876 100644 --- a/gcc/testsuite/gcc.dg/Walways-true-2.c +++ b/gcc/testsuite/gcc.dg/Walways-true-2.c @@ -9,7 +9,7 @@ extern int foo (int) __attribute__ ((weak)); -int i __attribute__ ((weak)); +extern int i __attribute__ ((weak)); void bar (int a) diff --git a/gcc/testsuite/gcc.dg/weak/weak-3.c b/gcc/testsuite/gcc.dg/weak/weak-3.c index a719848c935..ac85f42b34d 100644 --- a/gcc/testsuite/gcc.dg/weak/weak-3.c +++ b/gcc/testsuite/gcc.dg/weak/weak-3.c @@ -68,7 +68,9 @@ void * ffoox1g (void) { return (void *)0; } extern void * ffoo1g (void) __attribute__((weak, alias ("ffoox1g"))); void * foo1g (void) { - if (ffoo1g) + /* ffoo1g is a weak alias for a symbol defined in this file, expect + a -Waddress for the test (which is folded to true). */ + if (ffoo1g) // { dg-warning "-Waddress" } ffoo1g (); return 0; } ^ permalink raw reply [flat|nested] 14+ messages in thread
* PING [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-10-23 23:06 ` Martin Sebor @ 2021-11-07 23:31 ` Martin Sebor 2021-11-15 16:50 ` PING 2 " Martin Sebor 2021-11-16 20:23 ` Jason Merrill 1 sibling, 1 reply; 14+ messages in thread From: Martin Sebor @ 2021-11-07 23:31 UTC (permalink / raw) To: Jason Merrill, Joseph S. Myers; +Cc: gcc-patches Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582415.html On 10/23/21 5:06 PM, Martin Sebor wrote: > On 10/4/21 3:37 PM, Jason Merrill wrote: >> On 10/4/21 14:42, Martin Sebor wrote: >>> While resolving the recent -Waddress enhancement request (PR >>> PR102103) I came across a 2007 problem report about GCC 4 having >>> stopped warning for using the address of inline functions in >>> equality comparisons with null. With inline functions being >>> commonplace in C++ this seems like an important use case for >>> the warning. >>> >>> The change that resulted in suppressing the warning in these >>> cases was introduced inadvertently in a fix for PR 22252. >>> >>> To restore the warning, the attached patch enhances >>> the decl_with_nonnull_addr_p() function to return true also for >>> weak symbols for which a definition has been provided. >> >> I think you probably want to merge this function with >> fold-const.c:maybe_nonzero_address, which already handles more cases. > > maybe_nonzero_address() doesn't behave quite like > decl_with_nonnull_addr_p() expects and I'm reluctant to muck > around with the former too much since it's used for codegen, > while the latter just for warnings. (There is even a case > where the functions don't behave the same, and would result > in different warnings between C and C++ without some extra > help.) > > So in the attached revision I just have maybe_nonzero_address() > call decl_with_nonnull_addr_p() and then refine the failing > (or uncertain) cases separately, with some overlap between > them. > > Since I worked on this someone complained that some instances > of the warning newly enhanced under PR102103 aren't suppresed > in code resulting from macro expansion. Since it's trivial, > I include the fix for that report in this patch as well. > > Tested on x86_64-linux. > > Martin ^ permalink raw reply [flat|nested] 14+ messages in thread
* PING 2 [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-11-07 23:31 ` PING " Martin Sebor @ 2021-11-15 16:50 ` Martin Sebor 0 siblings, 0 replies; 14+ messages in thread From: Martin Sebor @ 2021-11-15 16:50 UTC (permalink / raw) To: Jason Merrill, Joseph S. Myers; +Cc: gcc-patches Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582415.html On 11/7/21 4:31 PM, Martin Sebor wrote: > Ping: > https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582415.html > > On 10/23/21 5:06 PM, Martin Sebor wrote: >> On 10/4/21 3:37 PM, Jason Merrill wrote: >>> On 10/4/21 14:42, Martin Sebor wrote: >>>> While resolving the recent -Waddress enhancement request (PR >>>> PR102103) I came across a 2007 problem report about GCC 4 having >>>> stopped warning for using the address of inline functions in >>>> equality comparisons with null. With inline functions being >>>> commonplace in C++ this seems like an important use case for >>>> the warning. >>>> >>>> The change that resulted in suppressing the warning in these >>>> cases was introduced inadvertently in a fix for PR 22252. >>>> >>>> To restore the warning, the attached patch enhances >>>> the decl_with_nonnull_addr_p() function to return true also for >>>> weak symbols for which a definition has been provided. >>> >>> I think you probably want to merge this function with >>> fold-const.c:maybe_nonzero_address, which already handles more cases. >> >> maybe_nonzero_address() doesn't behave quite like >> decl_with_nonnull_addr_p() expects and I'm reluctant to muck >> around with the former too much since it's used for codegen, >> while the latter just for warnings. (There is even a case >> where the functions don't behave the same, and would result >> in different warnings between C and C++ without some extra >> help.) >> >> So in the attached revision I just have maybe_nonzero_address() >> call decl_with_nonnull_addr_p() and then refine the failing >> (or uncertain) cases separately, with some overlap between >> them. >> >> Since I worked on this someone complained that some instances >> of the warning newly enhanced under PR102103 aren't suppresed >> in code resulting from macro expansion. Since it's trivial, >> I include the fix for that report in this patch as well. >> >> Tested on x86_64-linux. >> >> Martin > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-10-23 23:06 ` Martin Sebor 2021-11-07 23:31 ` PING " Martin Sebor @ 2021-11-16 20:23 ` Jason Merrill 2021-11-17 1:11 ` Martin Sebor 1 sibling, 1 reply; 14+ messages in thread From: Jason Merrill @ 2021-11-16 20:23 UTC (permalink / raw) To: Martin Sebor, Joseph S. Myers; +Cc: gcc-patches On 10/23/21 19:06, Martin Sebor wrote: > On 10/4/21 3:37 PM, Jason Merrill wrote: >> On 10/4/21 14:42, Martin Sebor wrote: >>> While resolving the recent -Waddress enhancement request (PR >>> PR102103) I came across a 2007 problem report about GCC 4 having >>> stopped warning for using the address of inline functions in >>> equality comparisons with null. With inline functions being >>> commonplace in C++ this seems like an important use case for >>> the warning. >>> >>> The change that resulted in suppressing the warning in these >>> cases was introduced inadvertently in a fix for PR 22252. >>> >>> To restore the warning, the attached patch enhances >>> the decl_with_nonnull_addr_p() function to return true also for >>> weak symbols for which a definition has been provided. >> >> I think you probably want to merge this function with >> fold-const.c:maybe_nonzero_address, which already handles more cases. > > maybe_nonzero_address() doesn't behave quite like > decl_with_nonnull_addr_p() expects and I'm reluctant to muck > around with the former too much since it's used for codegen, > while the latter just for warnings. (There is even a case > where the functions don't behave the same, and would result > in different warnings between C and C++ without some extra > help.) > > So in the attached revision I just have maybe_nonzero_address() > call decl_with_nonnull_addr_p() and then refine the failing > (or uncertain) cases separately, with some overlap between > them. > > Since I worked on this someone complained that some instances > of the warning newly enhanced under PR102103 aren't suppresed > in code resulting from macro expansion. Since it's trivial, > I include the fix for that report in this patch as well. > + allocated stroage might have a null address. */ typo. OK with that fixed. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-11-16 20:23 ` Jason Merrill @ 2021-11-17 1:11 ` Martin Sebor 2021-11-17 18:31 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Martin Sebor @ 2021-11-17 1:11 UTC (permalink / raw) To: Jason Merrill, Joseph S. Myers; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 3277 bytes --] On 11/16/21 1:23 PM, Jason Merrill wrote: > On 10/23/21 19:06, Martin Sebor wrote: >> On 10/4/21 3:37 PM, Jason Merrill wrote: >>> On 10/4/21 14:42, Martin Sebor wrote: >>>> While resolving the recent -Waddress enhancement request (PR >>>> PR102103) I came across a 2007 problem report about GCC 4 having >>>> stopped warning for using the address of inline functions in >>>> equality comparisons with null. With inline functions being >>>> commonplace in C++ this seems like an important use case for >>>> the warning. >>>> >>>> The change that resulted in suppressing the warning in these >>>> cases was introduced inadvertently in a fix for PR 22252. >>>> >>>> To restore the warning, the attached patch enhances >>>> the decl_with_nonnull_addr_p() function to return true also for >>>> weak symbols for which a definition has been provided. >>> >>> I think you probably want to merge this function with >>> fold-const.c:maybe_nonzero_address, which already handles more cases. >> >> maybe_nonzero_address() doesn't behave quite like >> decl_with_nonnull_addr_p() expects and I'm reluctant to muck >> around with the former too much since it's used for codegen, >> while the latter just for warnings. (There is even a case >> where the functions don't behave the same, and would result >> in different warnings between C and C++ without some extra >> help.) >> >> So in the attached revision I just have maybe_nonzero_address() >> call decl_with_nonnull_addr_p() and then refine the failing >> (or uncertain) cases separately, with some overlap between >> them. >> >> Since I worked on this someone complained that some instances >> of the warning newly enhanced under PR102103 aren't suppresed >> in code resulting from macro expansion. Since it's trivial, >> I include the fix for that report in this patch as well. > >> + allocated stroage might have a null address. */ > > typo. > > OK with that fixed. After retesting the patch before committing I noticed it triggers a regression in weak/weak-3.c that I missed the first time around. Here's the test case: extern void * ffoo1f (void); void * foo1f (void) { if (ffoo1f) /* { dg-warning "-Waddress" } */ ffoo1f (); return 0; } void * ffoox1f (void) { return (void *)0; } extern void * ffoo1f (void) __attribute__((weak, alias ("ffoox1f"))); The unexpected error is: a.c: At top level: a.c:1:15: error: ‘ffoo1f’ declared weak after being used 1 | extern void * ffoo1f (void); | ^~~~~~ The error is caused by the new call to maybe_nonzero_address() made from decl_with_nonnull_addr_p(). The call registers the symbol as used. So unless the error is desirable for this case I think it's best to go back to the originally proposed solution. I attach it for reference and will plan to commit it tomorrow unless I hear otherwise. Martin PS I don't know enough about the logic behind issuing this error in other situations to tell for sure that it's wrong in this one but I see no difference in the emitted code for a case in the same test that declares the alias first, before taking its address and that's accepted and this one. I also checked that both Clang and ICC accept the code either way, so I'm inclined to think the error would be a bug. [-- Attachment #2: gcc-33925.diff --] [-- Type: text/x-patch, Size: 14186 bytes --] Restore ancient -Waddress for weak symbols [PR33925]. Resolves: PR c/33925 - gcc -Waddress lost some useful warnings PR c/102867 - -Waddress from macro expansion in readelf.c gcc/c-family/ChangeLog: PR c++/33925 PR c/102867 * c-common.c (decl_with_nonnull_addr_p): Call maybe_nonzero_address and improve handling tof defined symbols. gcc/c/ChangeLog: PR c++/33925 PR c/102867 * c-typeck.c (maybe_warn_for_null_address): Suppress warnings for code resulting from macro expansion. gcc/cp/ChangeLog: PR c++/33925 PR c/102867 * typeck.c (warn_for_null_address): Suppress warnings for code resulting from macro expansion. gcc/ChangeLog: PR c++/33925 PR c/102867 * doc/invoke.texi (-Waddress): Update. gcc/testsuite/ChangeLog: PR c++/33925 PR c/102867 * g++.dg/warn/Walways-true-2.C: Adjust to avoid a valid warning. * c-c++-common/Waddress-5.c: New test. * c-c++-common/Waddress-6.c: New test. * g++.dg/warn/Waddress-7.C: New test. * g++.dg/warn/Walways-true-2.C: Adjust to avoid a valid warning. * gcc.dg/weak/weak-3.c: Expect a warning. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 436df45df68..5ab34c9eed8 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -3400,16 +3400,43 @@ c_wrap_maybe_const (tree expr, bool non_const) /* Return whether EXPR is a declaration whose address can never be NULL. The address of the first struct member could be NULL only if it were - accessed through a NULL pointer, and such an access would be invalid. */ + accessed through a NULL pointer, and such an access would be invalid. + The address of a weak symbol may be null unless it has a definition. */ bool decl_with_nonnull_addr_p (const_tree expr) { - return (DECL_P (expr) - && (TREE_CODE (expr) == FIELD_DECL - || TREE_CODE (expr) == PARM_DECL - || TREE_CODE (expr) == LABEL_DECL - || !DECL_WEAK (expr))); + if (!DECL_P (expr)) + return false; + + if (TREE_CODE (expr) == FIELD_DECL + || TREE_CODE (expr) == PARM_DECL + || TREE_CODE (expr) == LABEL_DECL) + return true; + + if (!VAR_OR_FUNCTION_DECL_P (expr)) + return false; + + if (!DECL_WEAK (expr)) + /* Ordinary (non-weak) symbols have nonnull addresses. */ + return true; + + if (DECL_INITIAL (expr) && DECL_INITIAL (expr) != error_mark_node) + /* Initialized weak symbols have nonnull addresses. */ + return true; + + if (DECL_EXTERNAL (expr) || !TREE_STATIC (expr)) + /* Uninitialized extern weak symbols and weak symbols with no + allocated storage might have a null address. */ + return false; + + tree attribs = DECL_ATTRIBUTES (expr); + if (lookup_attribute ("weakref", attribs)) + /* Weakref symbols might have a null address unless their referent + is known not to. Don't bother following weakref targets here. */ + return false; + + return true; } /* Prepare expr to be an argument of a TRUTH_NOT_EXPR, diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 782414f8c8c..50d70104766 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -11588,7 +11588,10 @@ build_vec_cmp (tree_code code, tree type, static void maybe_warn_for_null_address (location_t loc, tree op, tree_code code) { - if (!warn_address || warning_suppressed_p (op, OPT_Waddress)) + /* Prevent warnings issued for macro expansion. */ + if (!warn_address + || warning_suppressed_p (op, OPT_Waddress) + || from_macro_expansion_at (loc)) return; if (TREE_CODE (op) == NOP_EXPR) diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index cb20329ceb5..58919aaf13e 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -4594,9 +4594,11 @@ build_vec_cmp (tree_code code, tree type, static void warn_for_null_address (location_t location, tree op, tsubst_flags_t complain) { + /* Prevent warnings issued for macro expansion. */ if (!warn_address || (complain & tf_warning) == 0 || c_inhibit_evaluation_warnings != 0 + || from_macro_expansion_at (location) || warning_suppressed_p (op, OPT_Waddress)) return; diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 6070288856c..8a9559fa173 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -8649,6 +8649,8 @@ suppressed by casting the pointer operand to an integer type such as @code{inptr_t} or @code{uinptr_t}. Comparisons against string literals result in unspecified behavior and are not portable, and suggest the intent was to call @code{strcmp}. +The warning is suppressed if the suspicious expression is the result +of macro expansion. @option{-Waddress} warning is enabled by @option{-Wall}. @item -Wno-address-of-packed-member diff --git a/gcc/testsuite/c-c++-common/Waddress-5.c b/gcc/testsuite/c-c++-common/Waddress-5.c new file mode 100644 index 00000000000..5d63c7dae7c --- /dev/null +++ b/gcc/testsuite/c-c++-common/Waddress-5.c @@ -0,0 +1,133 @@ +/* PR c/33925 - missing -Waddress with the address of an inline function + { dg-do compile } + { dg-options "-Wall" } + { dg-require-weak "" } */ + +extern inline int eifn (void); +extern inline int eifn_def (void) { return 0; } + +static inline int sifn (void); +static inline int sifn_def (void) { return 0; } + +inline int ifn (void); +inline int ifn_def (void) { return 0; } + +extern __attribute__ ((weak)) int ewfn (void); +extern __attribute__ ((weak)) int ewfn_def (void) { return 0; } + +__attribute__ ((weak)) int wfn (void); +__attribute__ ((weak)) int wfn_def (void) { return 0; } + +static __attribute__((weakref ("ewfn"))) int swrfn (void); + +void test_function_eqz (int *p) +{ + *p++ = eifn == 0; // { dg-warning "-Waddress" } + *p++ = eifn_def == 0; // { dg-warning "-Waddress" } + *p++ = sifn == 0; // { dg-warning "-Waddress" } + *p++ = sifn_def == 0; // { dg-warning "-Waddress" } + *p++ = ifn == 0; // { dg-warning "-Waddress" } + *p++ = ifn_def == 0; // { dg-warning "-Waddress" } + *p++ = ewfn == 0; + *p++ = ewfn_def == 0; // { dg-warning "-Waddress" } + *p++ = wfn == 0; + *p++ = wfn_def == 0; // { dg-warning "-Waddress" } + *p++ = swrfn == 0; +} + + +int test_function_if (int i) +{ + if (eifn) // { dg-warning "-Waddress" } + i++; + if (eifn_def) // { dg-warning "-Waddress" } + i++; + if (sifn) // { dg-warning "-Waddress" } + i++; + if (sifn_def) // { dg-warning "-Waddress" } + i++; + if (ifn) // { dg-warning "-Waddress" } + i++; + if (ifn_def) // { dg-warning "-Waddress" } + i++; + if (ewfn) + i++; + if (ewfn_def) // { dg-warning "-Waddress" } + i++; + if (wfn) + i++; + if(wfn_def) // { dg-warning "-Waddress" } + i++; + if (swrfn) + i++; + return i; +} + + +extern int ei; +extern int ei_def = 1; + +static int si; +static int si_def = 1; + +int i; +int i_def = 1; + +extern __attribute__ ((weak)) int ewi; // declaration (may be null) +extern __attribute__ ((weak)) int ewi_def = 1; + +__attribute__ ((weak)) int wi; // definition (cannot be bull) +__attribute__ ((weak)) int wi_def = 1; + +static __attribute__((weakref ("ewi"))) int swri; + +void test_scalar (int *p) +{ + *p++ = &ei == 0; // { dg-warning "-Waddress" } + *p++ = &ei_def == 0; // { dg-warning "-Waddress" } + *p++ = &si == 0; // { dg-warning "-Waddress" } + *p++ = &si_def == 0; // { dg-warning "-Waddress" } + *p++ = &i == 0; // { dg-warning "-Waddress" } + *p++ = &i_def == 0; // { dg-warning "-Waddress" } + *p++ = &ewi == 0; + *p++ = &ewi_def == 0; // { dg-warning "-Waddress" } + *p++ = &wi == 0; // { dg-warning "-Waddress" } + *p++ = &wi_def == 0; // { dg-warning "-Waddress" } + *p++ = &swri == 0; +} + + +extern int eia[]; +extern int eia_def[] = { 1 }; + +static int sia[1]; +static int sia_def[1] = { 1 }; + +int ia[1]; +int ia_def[] = { 1 }; + +extern __attribute__ ((weak)) int ewia[]; +extern __attribute__ ((weak)) int ewia_def[] = { 1 }; + +__attribute__ ((weak)) int wia[1]; // definition (cannot be null) +__attribute__ ((weak)) int wia_def[] = { 1 }; + +static __attribute__((weakref ("ewia"))) int swria[1]; + +void test_array (int *p) +{ + *p++ = eia == 0; // { dg-warning "-Waddress" } + *p++ = eia_def == 0; // { dg-warning "-Waddress" } + *p++ = sia == 0; // { dg-warning "-Waddress" } + *p++ = sia_def == 0; // { dg-warning "-Waddress" } + *p++ = ia == 0; // { dg-warning "-Waddress" } + *p++ = ia_def == 0; // { dg-warning "-Waddress" } + *p++ = ewia == 0; + *p++ = ewia_def == 0; // { dg-warning "-Waddress" } + *p++ = wia == 0; // { dg-warning "-Waddress" } + *p++ = wia_def == 0; // { dg-warning "-Waddress" } + *p++ = swria == 0; +} + +/* { dg-prune-output "never defined" } + { dg-prune-output "initialized and declared 'extern'" } */ diff --git a/gcc/testsuite/c-c++-common/Waddress-6.c b/gcc/testsuite/c-c++-common/Waddress-6.c new file mode 100644 index 00000000000..e66e6e4a0b0 --- /dev/null +++ b/gcc/testsuite/c-c++-common/Waddress-6.c @@ -0,0 +1,32 @@ +/* PR c/102867 - -Waddress from macro expansion in readelf.c + { dg-do compile } + { dg-options "-Wall" } */ + +#define F(x) ((&x) != 0) + +int warn_nomacro (int *p, int i) +{ + return &p[i] != 0; // { dg-warning "-Waddress" } +} + +int nowarn_macro_expansion (int *p, int i) +{ + // Verify that -Waddress isn't issued for code from macro expansion. + return F (p[i]); // { dg-bogus "-Waddress" } +} + +#define G(x, i) ((&x) + i) + +int warn_function_macro_expansion (int *p, int i) +{ + /* Verify that -Waddress is issued for code involving macro expansion + where the comparison takes place outside the macro. */ + return G (*p, i) != 0; // { dg-warning "-Waddress" } +} + +#define malloc __builtin_malloc + +int warn_object_macro_expansion (int *p, int i) +{ + return malloc != 0; // { dg-warning "-Waddress" } +} diff --git a/gcc/testsuite/g++.dg/warn/Waddress-7.C b/gcc/testsuite/g++.dg/warn/Waddress-7.C new file mode 100644 index 00000000000..efdafa50cd1 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Waddress-7.C @@ -0,0 +1,76 @@ +/* PR c/33925 - missing -Waddress with the address of an inline function + { dg-do compile } + { dg-options "-Wall" } */ + +struct A +{ + int mf (); + int mf_def () { return 0; } + + static int smf (); + static int smf_def () { return 0; } + + int mi; + static int smi; + static int smi_def; + + __attribute__ ((weak)) static int wsmi; + __attribute__ ((weak)) static int wsmi_def; + + int mia[]; + static int smia[]; + static int smia_def[]; + + __attribute__ ((weak)) static int wsmia[]; + __attribute__ ((weak)) static int wsmia_def[]; + + void test_waddress (bool*); +}; + + +/* __attribute__ ((weak)) static */ int A::smi_def = 0; +/* __attribute__ ((weak)) static */ int A::smia_def[] = { 0 }; + +/* __attribute__ ((weak)) static */ int A::wsmi_def = 0; +/* __attribute__ ((weak)) static */ int A::wsmia_def[] = { 0 }; + + + +void A::test_waddress (bool *p) +{ + if (mf) // { dg-error "cannot convert" } + p++; + if (mf_def) // { dg-error "cannot convert" } + p++; + + if (smf) // { dg-warning "-Waddress" } + p++; + if (smf_def) // { dg-warning "-Waddress" } + p++; + + if (&mi) // { dg-warning "-Waddress" } + p++; + if (&smi) // { dg-warning "-Waddress" } + p++; + if (&smi_def) // { dg-warning "-Waddress" } + p++; + + if (&wsmi) + p++; + + if (&wsmi_def) // { dg-warning "-Waddress" } + p++; + + if (mia) // { dg-warning "-Waddress" } + p++; + if (smia) // { dg-warning "-Waddress" } + p++; + if (smia_def) // { dg-warning "-Waddress" } + p++; + + if (wsmia) + p++; + + if (wsmia_def) // { dg-warning "-Waddress" } + p++; +} diff --git a/gcc/testsuite/g++.dg/warn/Walways-true-2.C b/gcc/testsuite/g++.dg/warn/Walways-true-2.C index 29a80e5c113..e951e95b336 100644 --- a/gcc/testsuite/g++.dg/warn/Walways-true-2.C +++ b/gcc/testsuite/g++.dg/warn/Walways-true-2.C @@ -9,7 +9,7 @@ extern int foo (int) __attribute__ ((weak)); -int i __attribute__ ((weak)); +extern int i __attribute__ ((weak)); void bar (int a) diff --git a/gcc/testsuite/gcc.dg/Walways-true-2.c b/gcc/testsuite/gcc.dg/Walways-true-2.c index 7f0bb7b10b9..ae3262b6876 100644 --- a/gcc/testsuite/gcc.dg/Walways-true-2.c +++ b/gcc/testsuite/gcc.dg/Walways-true-2.c @@ -9,7 +9,7 @@ extern int foo (int) __attribute__ ((weak)); -int i __attribute__ ((weak)); +extern int i __attribute__ ((weak)); void bar (int a) diff --git a/gcc/testsuite/gcc.dg/weak/weak-3.c b/gcc/testsuite/gcc.dg/weak/weak-3.c index a719848c935..5fdf029cf35 100644 --- a/gcc/testsuite/gcc.dg/weak/weak-3.c +++ b/gcc/testsuite/gcc.dg/weak/weak-3.c @@ -55,7 +55,7 @@ void * foo1e (void) extern void * ffoo1f (void); void * foo1f (void) { - if (ffoo1f) /* { dg-warning "" } */ + if (ffoo1f) /* { dg-warning "-Waddress" } */ ffoo1f (); return 0; } @@ -68,7 +68,9 @@ void * ffoox1g (void) { return (void *)0; } extern void * ffoo1g (void) __attribute__((weak, alias ("ffoox1g"))); void * foo1g (void) { - if (ffoo1g) + /* ffoo1g is a weak alias for a symbol defined in this file, expect + a -Waddress for the test (which is folded to true). */ + if (ffoo1g) // { dg-warning "-Waddress" } ffoo1g (); return 0; } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-11-17 1:11 ` Martin Sebor @ 2021-11-17 18:31 ` Jason Merrill 2021-11-17 19:21 ` Martin Sebor 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2021-11-17 18:31 UTC (permalink / raw) To: Martin Sebor, Joseph S. Myers; +Cc: gcc-patches On 11/16/21 20:11, Martin Sebor wrote: > On 11/16/21 1:23 PM, Jason Merrill wrote: >> On 10/23/21 19:06, Martin Sebor wrote: >>> On 10/4/21 3:37 PM, Jason Merrill wrote: >>>> On 10/4/21 14:42, Martin Sebor wrote: >>>>> While resolving the recent -Waddress enhancement request (PR >>>>> PR102103) I came across a 2007 problem report about GCC 4 having >>>>> stopped warning for using the address of inline functions in >>>>> equality comparisons with null. With inline functions being >>>>> commonplace in C++ this seems like an important use case for >>>>> the warning. >>>>> >>>>> The change that resulted in suppressing the warning in these >>>>> cases was introduced inadvertently in a fix for PR 22252. >>>>> >>>>> To restore the warning, the attached patch enhances >>>>> the decl_with_nonnull_addr_p() function to return true also for >>>>> weak symbols for which a definition has been provided. >>>> >>>> I think you probably want to merge this function with >>>> fold-const.c:maybe_nonzero_address, which already handles more cases. >>> >>> maybe_nonzero_address() doesn't behave quite like >>> decl_with_nonnull_addr_p() expects and I'm reluctant to muck >>> around with the former too much since it's used for codegen, >>> while the latter just for warnings. (There is even a case >>> where the functions don't behave the same, and would result >>> in different warnings between C and C++ without some extra >>> help.) >>> >>> So in the attached revision I just have maybe_nonzero_address() >>> call decl_with_nonnull_addr_p() and then refine the failing >>> (or uncertain) cases separately, with some overlap between >>> them. >>> >>> Since I worked on this someone complained that some instances >>> of the warning newly enhanced under PR102103 aren't suppresed >>> in code resulting from macro expansion. Since it's trivial, >>> I include the fix for that report in this patch as well. >> >>> + allocated stroage might have a null address. */ >> >> typo. >> >> OK with that fixed. > > After retesting the patch before committing I noticed it triggers > a regression in weak/weak-3.c that I missed the first time around. > Here's the test case: > > extern void * ffoo1f (void); > void * foo1f (void) > { > if (ffoo1f) /* { dg-warning "-Waddress" } */ > ffoo1f (); > return 0; > } > > void * ffoox1f (void) { return (void *)0; } > extern void * ffoo1f (void) __attribute__((weak, alias ("ffoox1f"))); > > The unexpected error is: > > a.c: At top level: > a.c:1:15: error: ‘ffoo1f’ declared weak after being used > 1 | extern void * ffoo1f (void); > | ^~~~~~ > > The error is caused by the new call to maybe_nonzero_address() > made from decl_with_nonnull_addr_p(). The call registers > the symbol as used. > > So unless the error is desirable for this case I think it's > best to go back to the originally proposed solution. I attach > it for reference and will plan to commit it tomorrow unless I > hear otherwise. Hmm, the error seems correct to me: we tested whether the address is nonzero in the dg-warning line, and presumably evaluating that test could depend on the absence of weak. > PS I don't know enough about the logic behind issuing this error > in other situations to tell for sure that it's wrong in this one > but I see no difference in the emitted code for a case in the same > test that declares the alias first, before taking its address and > that's accepted and this one. I also checked that both Clang and > ICC accept the code either way, so I'm inclined to think the error > would be a bug. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-11-17 18:31 ` Jason Merrill @ 2021-11-17 19:21 ` Martin Sebor 2021-11-18 1:27 ` Martin Sebor 0 siblings, 1 reply; 14+ messages in thread From: Martin Sebor @ 2021-11-17 19:21 UTC (permalink / raw) To: Jason Merrill, Joseph S. Myers; +Cc: gcc-patches On 11/17/21 11:31 AM, Jason Merrill wrote: > On 11/16/21 20:11, Martin Sebor wrote: >> On 11/16/21 1:23 PM, Jason Merrill wrote: >>> On 10/23/21 19:06, Martin Sebor wrote: >>>> On 10/4/21 3:37 PM, Jason Merrill wrote: >>>>> On 10/4/21 14:42, Martin Sebor wrote: >>>>>> While resolving the recent -Waddress enhancement request (PR >>>>>> PR102103) I came across a 2007 problem report about GCC 4 having >>>>>> stopped warning for using the address of inline functions in >>>>>> equality comparisons with null. With inline functions being >>>>>> commonplace in C++ this seems like an important use case for >>>>>> the warning. >>>>>> >>>>>> The change that resulted in suppressing the warning in these >>>>>> cases was introduced inadvertently in a fix for PR 22252. >>>>>> >>>>>> To restore the warning, the attached patch enhances >>>>>> the decl_with_nonnull_addr_p() function to return true also for >>>>>> weak symbols for which a definition has been provided. >>>>> >>>>> I think you probably want to merge this function with >>>>> fold-const.c:maybe_nonzero_address, which already handles more cases. >>>> >>>> maybe_nonzero_address() doesn't behave quite like >>>> decl_with_nonnull_addr_p() expects and I'm reluctant to muck >>>> around with the former too much since it's used for codegen, >>>> while the latter just for warnings. (There is even a case >>>> where the functions don't behave the same, and would result >>>> in different warnings between C and C++ without some extra >>>> help.) >>>> >>>> So in the attached revision I just have maybe_nonzero_address() >>>> call decl_with_nonnull_addr_p() and then refine the failing >>>> (or uncertain) cases separately, with some overlap between >>>> them. >>>> >>>> Since I worked on this someone complained that some instances >>>> of the warning newly enhanced under PR102103 aren't suppresed >>>> in code resulting from macro expansion. Since it's trivial, >>>> I include the fix for that report in this patch as well. >>> >>>> + allocated stroage might have a null address. */ >>> >>> typo. >>> >>> OK with that fixed. >> >> After retesting the patch before committing I noticed it triggers >> a regression in weak/weak-3.c that I missed the first time around. >> Here's the test case: >> >> extern void * ffoo1f (void); >> void * foo1f (void) >> { >> if (ffoo1f) /* { dg-warning "-Waddress" } */ >> ffoo1f (); >> return 0; >> } >> >> void * ffoox1f (void) { return (void *)0; } >> extern void * ffoo1f (void) __attribute__((weak, alias ("ffoox1f"))); >> >> The unexpected error is: >> >> a.c: At top level: >> a.c:1:15: error: ‘ffoo1f’ declared weak after being used >> 1 | extern void * ffoo1f (void); >> | ^~~~~~ >> >> The error is caused by the new call to maybe_nonzero_address() >> made from decl_with_nonnull_addr_p(). The call registers >> the symbol as used. >> >> So unless the error is desirable for this case I think it's >> best to go back to the originally proposed solution. I attach >> it for reference and will plan to commit it tomorrow unless I >> hear otherwise. > > Hmm, the error seems correct to me: we tested whether the address is > nonzero in the dg-warning line, and presumably evaluating that test > could depend on the absence of weak. Sorry, I don't know enough yet to judge this. Since the error is unrelated to what I'm fixing I would prefer not to introduce it in the same patch. I'm happy to open a separate bug for the missing error for the test case above, look some more into why it isn't issued, and if it's decided the error is intended either add the call back to trigger it or do whatever else may be more appropriate). Are you okay with me going ahead and committing the most recent patch as is? If not, do you want me to commit the previous version and change the weak-3.c test to expect the error? Martin > >> PS I don't know enough about the logic behind issuing this error >> in other situations to tell for sure that it's wrong in this one >> but I see no difference in the emitted code for a case in the same >> test that declares the alias first, before taking its address and >> that's accepted and this one. I also checked that both Clang and >> ICC accept the code either way, so I'm inclined to think the error >> would be a bug. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-11-17 19:21 ` Martin Sebor @ 2021-11-18 1:27 ` Martin Sebor 2021-11-18 15:58 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Martin Sebor @ 2021-11-18 1:27 UTC (permalink / raw) To: Jason Merrill, Joseph S. Myers; +Cc: gcc-patches On 11/17/21 12:21 PM, Martin Sebor wrote: > On 11/17/21 11:31 AM, Jason Merrill wrote: >> On 11/16/21 20:11, Martin Sebor wrote: >>> On 11/16/21 1:23 PM, Jason Merrill wrote: >>>> On 10/23/21 19:06, Martin Sebor wrote: >>>>> On 10/4/21 3:37 PM, Jason Merrill wrote: >>>>>> On 10/4/21 14:42, Martin Sebor wrote: >>>>>>> While resolving the recent -Waddress enhancement request (PR >>>>>>> PR102103) I came across a 2007 problem report about GCC 4 having >>>>>>> stopped warning for using the address of inline functions in >>>>>>> equality comparisons with null. With inline functions being >>>>>>> commonplace in C++ this seems like an important use case for >>>>>>> the warning. >>>>>>> >>>>>>> The change that resulted in suppressing the warning in these >>>>>>> cases was introduced inadvertently in a fix for PR 22252. >>>>>>> >>>>>>> To restore the warning, the attached patch enhances >>>>>>> the decl_with_nonnull_addr_p() function to return true also for >>>>>>> weak symbols for which a definition has been provided. >>>>>> >>>>>> I think you probably want to merge this function with >>>>>> fold-const.c:maybe_nonzero_address, which already handles more cases. >>>>> >>>>> maybe_nonzero_address() doesn't behave quite like >>>>> decl_with_nonnull_addr_p() expects and I'm reluctant to muck >>>>> around with the former too much since it's used for codegen, >>>>> while the latter just for warnings. (There is even a case >>>>> where the functions don't behave the same, and would result >>>>> in different warnings between C and C++ without some extra >>>>> help.) >>>>> >>>>> So in the attached revision I just have maybe_nonzero_address() >>>>> call decl_with_nonnull_addr_p() and then refine the failing >>>>> (or uncertain) cases separately, with some overlap between >>>>> them. >>>>> >>>>> Since I worked on this someone complained that some instances >>>>> of the warning newly enhanced under PR102103 aren't suppresed >>>>> in code resulting from macro expansion. Since it's trivial, >>>>> I include the fix for that report in this patch as well. >>>> >>>>> + allocated stroage might have a null address. */ >>>> >>>> typo. >>>> >>>> OK with that fixed. >>> >>> After retesting the patch before committing I noticed it triggers >>> a regression in weak/weak-3.c that I missed the first time around. >>> Here's the test case: >>> >>> extern void * ffoo1f (void); >>> void * foo1f (void) >>> { >>> if (ffoo1f) /* { dg-warning "-Waddress" } */ >>> ffoo1f (); >>> return 0; >>> } >>> >>> void * ffoox1f (void) { return (void *)0; } >>> extern void * ffoo1f (void) __attribute__((weak, alias ("ffoox1f"))); >>> >>> The unexpected error is: >>> >>> a.c: At top level: >>> a.c:1:15: error: ‘ffoo1f’ declared weak after being used >>> 1 | extern void * ffoo1f (void); >>> | ^~~~~~ >>> >>> The error is caused by the new call to maybe_nonzero_address() >>> made from decl_with_nonnull_addr_p(). The call registers >>> the symbol as used. >>> >>> So unless the error is desirable for this case I think it's >>> best to go back to the originally proposed solution. I attach >>> it for reference and will plan to commit it tomorrow unless I >>> hear otherwise. >> >> Hmm, the error seems correct to me: we tested whether the address is >> nonzero in the dg-warning line, and presumably evaluating that test >> could depend on the absence of weak. > > Sorry, I don't know enough yet to judge this. I've created a test case involving just a weak symbol (no alias) that shows that the front end folds to true a test of the address of a symbol subsequently declared weak: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103310 Clang and ICC do the same thing here; only Clang and GCC issue a warning that the inequality is folded to true (here's a live link to it: https://godbolt.org/z/a8Tx9Psee). This doesn't seem ideal but I wouldn't call it a serious problem. The case in weak-3.c is different: there the weak symbol is an alias for a locally defined function. There, the alias cannot become null and so folding the test is safe and giving an error for it would be a regression. I would tend to view issuing a hard error in this case a more serious problem than the first (especially after reading the discussion below), but YMMV. The weak-3.c test was added along with a fix for PR 6343. Here's a discussion of the problem: https://gcc.gnu.org/legacy-ml/gcc/2002-04/msg00838.html Please let me know which of the alternatives below you prefer or if you want something else. > Since the error is unrelated to what I'm fixing I would prefer > not to introduce it in the same patch. I'm happy to open > a separate bug for the missing error for the test case above, > look some more into why it isn't issued, and if it's decided > the error is intended either add the call back to trigger it > or do whatever else may be more appropriate). > > Are you okay with me going ahead and committing the most recent > patch as is? > > If not, do you want me to commit the previous version and change > the weak-3.c test to expect the error? > > Martin > >> >>> PS I don't know enough about the logic behind issuing this error >>> in other situations to tell for sure that it's wrong in this one >>> but I see no difference in the emitted code for a case in the same >>> test that declares the alias first, before taking its address and >>> that's accepted and this one. I also checked that both Clang and >>> ICC accept the code either way, so I'm inclined to think the error >>> would be a bug. >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-11-18 1:27 ` Martin Sebor @ 2021-11-18 15:58 ` Jason Merrill 0 siblings, 0 replies; 14+ messages in thread From: Jason Merrill @ 2021-11-18 15:58 UTC (permalink / raw) To: Martin Sebor, Joseph S. Myers; +Cc: gcc-patches, Jan Hubicka On 11/17/21 20:27, Martin Sebor wrote: > On 11/17/21 12:21 PM, Martin Sebor wrote: >> On 11/17/21 11:31 AM, Jason Merrill wrote: >>> On 11/16/21 20:11, Martin Sebor wrote: >>>> On 11/16/21 1:23 PM, Jason Merrill wrote: >>>>> On 10/23/21 19:06, Martin Sebor wrote: >>>>>> On 10/4/21 3:37 PM, Jason Merrill wrote: >>>>>>> On 10/4/21 14:42, Martin Sebor wrote: >>>>>>>> While resolving the recent -Waddress enhancement request (PR >>>>>>>> PR102103) I came across a 2007 problem report about GCC 4 having >>>>>>>> stopped warning for using the address of inline functions in >>>>>>>> equality comparisons with null. With inline functions being >>>>>>>> commonplace in C++ this seems like an important use case for >>>>>>>> the warning. >>>>>>>> >>>>>>>> The change that resulted in suppressing the warning in these >>>>>>>> cases was introduced inadvertently in a fix for PR 22252. >>>>>>>> >>>>>>>> To restore the warning, the attached patch enhances >>>>>>>> the decl_with_nonnull_addr_p() function to return true also for >>>>>>>> weak symbols for which a definition has been provided. >>>>>>> >>>>>>> I think you probably want to merge this function with >>>>>>> fold-const.c:maybe_nonzero_address, which already handles more >>>>>>> cases. >>>>>> >>>>>> maybe_nonzero_address() doesn't behave quite like >>>>>> decl_with_nonnull_addr_p() expects and I'm reluctant to muck >>>>>> around with the former too much since it's used for codegen, >>>>>> while the latter just for warnings. (There is even a case >>>>>> where the functions don't behave the same, and would result >>>>>> in different warnings between C and C++ without some extra >>>>>> help.) >>>>>> >>>>>> So in the attached revision I just have maybe_nonzero_address() >>>>>> call decl_with_nonnull_addr_p() and then refine the failing >>>>>> (or uncertain) cases separately, with some overlap between >>>>>> them. >>>>>> >>>>>> Since I worked on this someone complained that some instances >>>>>> of the warning newly enhanced under PR102103 aren't suppresed >>>>>> in code resulting from macro expansion. Since it's trivial, >>>>>> I include the fix for that report in this patch as well. >>>>> >>>>>> + allocated stroage might have a null address. */ >>>>> >>>>> typo. >>>>> >>>>> OK with that fixed. >>>> >>>> After retesting the patch before committing I noticed it triggers >>>> a regression in weak/weak-3.c that I missed the first time around. >>>> Here's the test case: >>>> >>>> extern void * ffoo1f (void); >>>> void * foo1f (void) >>>> { >>>> if (ffoo1f) /* { dg-warning "-Waddress" } */ >>>> ffoo1f (); >>>> return 0; >>>> } >>>> >>>> void * ffoox1f (void) { return (void *)0; } >>>> extern void * ffoo1f (void) __attribute__((weak, alias ("ffoox1f"))); >>>> >>>> The unexpected error is: >>>> >>>> a.c: At top level: >>>> a.c:1:15: error: ‘ffoo1f’ declared weak after being used >>>> 1 | extern void * ffoo1f (void); >>>> | ^~~~~~ >>>> >>>> The error is caused by the new call to maybe_nonzero_address() >>>> made from decl_with_nonnull_addr_p(). The call registers >>>> the symbol as used. >>>> >>>> So unless the error is desirable for this case I think it's >>>> best to go back to the originally proposed solution. I attach >>>> it for reference and will plan to commit it tomorrow unless I >>>> hear otherwise. >>> >>> Hmm, the error seems correct to me: we tested whether the address is >>> nonzero in the dg-warning line, and presumably evaluating that test >>> could depend on the absence of weak. >> >> Sorry, I don't know enough yet to judge this. > > I've created a test case involving just a weak symbol (no alias) > that shows that the front end folds to true a test of the address > of a symbol subsequently declared weak: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103310 > > Clang and ICC do the same thing here; only Clang and GCC issue > a warning that the inequality is folded to true (here's a live > link to it: https://godbolt.org/z/a8Tx9Psee). > > This doesn't seem ideal but I wouldn't call it a serious problem. > > The case in weak-3.c is different: there the weak symbol is > an alias for a locally defined function. There, the alias cannot > become null and so folding the test is safe and giving an error > for it would be a regression. I would tend to view issuing > a hard error in this case a more serious problem than the first > (especially after reading the discussion below), but YMMV. Ah, very good point. This issue seems to go back to Honza's r5-3627, which changed symtab_node::get to symtab_node::get_create in the code that became maybe_nonzero_address, so that we decide early whether a particular function is weak or not. This was done so that constant-evaluation could properly decide that the address of a function is non-null. But it's harmful when we do that for speculative folding; we should only return a definitive answer, and set refuse_visibility_changes, when a constant result is required. I guess we need a way to tell fold that we really want a constant value, have the C++ front end set that for manifestly-constant-evaluated expressions, and only use get_create in that case. > The weak-3.c test was added along with a fix for PR 6343. > Here's a discussion of the problem: > https://gcc.gnu.org/legacy-ml/gcc/2002-04/msg00838.html > > Please let me know which of the alternatives below you prefer > or if you want something else. > >> Since the error is unrelated to what I'm fixing I would prefer >> not to introduce it in the same patch. I'm happy to open >> a separate bug for the missing error for the test case above, >> look some more into why it isn't issued, and if it's decided >> the error is intended either add the call back to trigger it >> or do whatever else may be more appropriate). >> >> Are you okay with me going ahead and committing the most recent >> patch as is? OK. I'll copy my analysis above into PR103310. >> If not, do you want me to commit the previous version and change >> the weak-3.c test to expect the error? >> >> Martin >> >>> >>>> PS I don't know enough about the logic behind issuing this error >>>> in other situations to tell for sure that it's wrong in this one >>>> but I see no difference in the emitted code for a case in the same >>>> test that declares the alias first, before taking its address and >>>> that's accepted and this one. I also checked that both Clang and >>>> ICC accept the code either way, so I'm inclined to think the error >>>> would be a bug. >>> >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-10-04 18:42 [PATCH] restore ancient -Waddress for weak symbols [PR33925] Martin Sebor 2021-10-04 21:37 ` Jason Merrill @ 2021-10-04 22:39 ` Eric Gallager 2021-11-02 18:51 ` Martin Sebor 1 sibling, 1 reply; 14+ messages in thread From: Eric Gallager @ 2021-10-04 22:39 UTC (permalink / raw) To: Martin Sebor; +Cc: Joseph S. Myers, Jason Merrill, gcc-patches On Mon, Oct 4, 2021 at 2:43 PM Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > While resolving the recent -Waddress enhancement request (PR > PR102103) I came across a 2007 problem report about GCC 4 having > stopped warning for using the address of inline functions in > equality comparisons with null. With inline functions being > commonplace in C++ this seems like an important use case for > the warning. > > The change that resulted in suppressing the warning in these > cases was introduced inadvertently in a fix for PR 22252. > > To restore the warning, the attached patch enhances > the decl_with_nonnull_addr_p() function to return true also for > weak symbols for which a definition has been provided. > > Tested on x86_64-linux and by comparing the GCC output for new > test cases to Clang's which diagnoses all but one instance of > these cases with either -Wtautological-pointer-compare or > -Wpointer-bool-conversion, depending on context. Would it make sense to use the same names as clang's flags here, too, instead of dumping them all under -Waddress? I think the additional granularity could be helpful for people who only want some warnings, but not others. > The one case where Clang doesn't issue a warning but GCC > with the patch does is for a symbol explicitly declared with > attribute weak for which a definition has been provided. > I believe the address of such symbols is necessarily nonnull and > so issuing the warning is helpful > (both GCC and Clang fold such comparisons to a constant). > > Martin ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-10-04 22:39 ` Eric Gallager @ 2021-11-02 18:51 ` Martin Sebor 2021-11-02 19:28 ` Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Martin Sebor @ 2021-11-02 18:51 UTC (permalink / raw) To: Eric Gallager; +Cc: Joseph S. Myers, Jason Merrill, gcc-patches On 10/4/21 4:39 PM, Eric Gallager wrote: > On Mon, Oct 4, 2021 at 2:43 PM Martin Sebor via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> While resolving the recent -Waddress enhancement request (PR >> PR102103) I came across a 2007 problem report about GCC 4 having >> stopped warning for using the address of inline functions in >> equality comparisons with null. With inline functions being >> commonplace in C++ this seems like an important use case for >> the warning. >> >> The change that resulted in suppressing the warning in these >> cases was introduced inadvertently in a fix for PR 22252. >> >> To restore the warning, the attached patch enhances >> the decl_with_nonnull_addr_p() function to return true also for >> weak symbols for which a definition has been provided. >> >> Tested on x86_64-linux and by comparing the GCC output for new >> test cases to Clang's which diagnoses all but one instance of >> these cases with either -Wtautological-pointer-compare or >> -Wpointer-bool-conversion, depending on context. > > Would it make sense to use the same names as clang's flags here, too, > instead of dumping them all under -Waddress? I think the additional > granularity could be helpful for people who only want some warnings, > but not others. In general I agree. In this case I'm not sure. The options that control these warnings in neither compiler make perfect sense to me. Here's a breakdown of the cases: Clang GCC array == array -Wtautological-compare -Warray-compare &decl == null -Wtautological-pointer-compare -Waddress &decl1 == &decl2 N/A N/A GCC has recently diverged from Clang by introducing the new -Warray-compare option, and we don't have -Wtautological-pointer-compare. So while I think it makes sense to use the same names for new features as those they are controlled by in Clang, the argument to do the same for simple enhancements to existing features is quite a bit less compelling. We'd likely end up diagnosing different subsets of the same problem under different options. Martin > >> The one case where Clang doesn't issue a warning but GCC >> with the patch does is for a symbol explicitly declared with >> attribute weak for which a definition has been provided. >> I believe the address of such symbols is necessarily nonnull and >> so issuing the warning is helpful >> (both GCC and Clang fold such comparisons to a constant). >> >> Martin ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] 2021-11-02 18:51 ` Martin Sebor @ 2021-11-02 19:28 ` Marek Polacek 0 siblings, 0 replies; 14+ messages in thread From: Marek Polacek @ 2021-11-02 19:28 UTC (permalink / raw) To: Martin Sebor; +Cc: Eric Gallager, gcc-patches, Joseph S. Myers On Tue, Nov 02, 2021 at 12:51:16PM -0600, Martin Sebor via Gcc-patches wrote: > On 10/4/21 4:39 PM, Eric Gallager wrote: > > On Mon, Oct 4, 2021 at 2:43 PM Martin Sebor via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > > > > > > While resolving the recent -Waddress enhancement request (PR > > > PR102103) I came across a 2007 problem report about GCC 4 having > > > stopped warning for using the address of inline functions in > > > equality comparisons with null. With inline functions being > > > commonplace in C++ this seems like an important use case for > > > the warning. > > > > > > The change that resulted in suppressing the warning in these > > > cases was introduced inadvertently in a fix for PR 22252. > > > > > > To restore the warning, the attached patch enhances > > > the decl_with_nonnull_addr_p() function to return true also for > > > weak symbols for which a definition has been provided. > > > > > > Tested on x86_64-linux and by comparing the GCC output for new > > > test cases to Clang's which diagnoses all but one instance of > > > these cases with either -Wtautological-pointer-compare or > > > -Wpointer-bool-conversion, depending on context. > > > > Would it make sense to use the same names as clang's flags here, too, > > instead of dumping them all under -Waddress? I think the additional > > granularity could be helpful for people who only want some warnings, > > but not others. > > In general I agree. In this case I'm not sure. The options > that control these warnings in neither compiler make perfect > sense to me. Here's a breakdown of the cases: > > Clang GCC > array == array -Wtautological-compare -Warray-compare > &decl == null -Wtautological-pointer-compare -Waddress > &decl1 == &decl2 N/A N/A > > GCC has recently diverged from Clang by introducing the new > -Warray-compare option, and we don't have That's not exactly true: -Warray-compare is not meant to be -Wtautological-compare. I introduced -Warray-compare because the C++ standard now says that array == array is deprecated, but -Wtautological-compare comes from a different angle: it warns because the comparison always evaluates to a constant. > -Wtautological-pointer-compare. So while I think it makes > sense to use the same names for new features as those they > are controlled by in Clang, the argument to do the same for > simple enhancements to existing features is quite a bit less > compelling. We'd likely end up diagnosing different subsets > of the same problem under different options. Yeah, in this particular case I think it'd be better to simple enhance -Waddress rather than to invent a new option. Marek ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-11-18 15:58 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-04 18:42 [PATCH] restore ancient -Waddress for weak symbols [PR33925] Martin Sebor 2021-10-04 21:37 ` Jason Merrill 2021-10-23 23:06 ` Martin Sebor 2021-11-07 23:31 ` PING " Martin Sebor 2021-11-15 16:50 ` PING 2 " Martin Sebor 2021-11-16 20:23 ` Jason Merrill 2021-11-17 1:11 ` Martin Sebor 2021-11-17 18:31 ` Jason Merrill 2021-11-17 19:21 ` Martin Sebor 2021-11-18 1:27 ` Martin Sebor 2021-11-18 15:58 ` Jason Merrill 2021-10-04 22:39 ` Eric Gallager 2021-11-02 18:51 ` Martin Sebor 2021-11-02 19:28 ` Marek Polacek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).