From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52e.google.com (mail-pg1-x52e.google.com [IPv6:2607:f8b0:4864:20::52e]) by sourceware.org (Postfix) with ESMTPS id 65C11385AC38 for ; Wed, 17 Nov 2021 01:11:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 65C11385AC38 Received: by mail-pg1-x52e.google.com with SMTP id r23so672586pgu.13 for ; Tue, 16 Nov 2021 17:11:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=UgOo1Y9DWp5cq5vVBg61bwz+p69rcPS3gVtKu0vg6FU=; b=KgUBCCCi73iD+imksgDSHaPXlXn2wgk/fiMiA/QMmo0u/hS0RkgMayNTNcQWR1KtvG 3+i8xIohZt4qBGsZB9N9E0+dDKW5mGhdMdt/xh3ZwHwtXS2IljaHxbHxsUd4kzEkvWPJ 0In8t6Sb7V3JVUW3EFbfDQ8CO0KGLuWb+hYGy58oItGOeB0DAAVFV51RmatkIKDq9R9O GZ6aK5vulsVX7IJ9MAH+YzSNMkcmjamkXwEBhwze+LDxzackWWADFfobIM1AyAUVRCya V86BFthgCqUqcTVS5P/MHfcvnLr2++RzVT/ldQo6qLmwsDqrtCyJ2tmlxEZwSfzdNHv5 6u0A== X-Gm-Message-State: AOAM531qMUE/qAEe1eoLkPV1JR1Mx6iTTTgz/aIBfcKm1Ggz2QbuyK+o kcWH9tJTWZYtnWjmo5Z/1tl6xcM7djw= X-Google-Smtp-Source: ABdhPJxrYbPAsw9ifMj7z25zIQqtH0HrB12JZdh85AmezNDwGMV9mTWTnZz1PeYTEyirtFP1MRs+Pw== X-Received: by 2002:a63:551b:: with SMTP id j27mr2448599pgb.453.1637111466326; Tue, 16 Nov 2021 17:11:06 -0800 (PST) Received: from [192.168.0.41] (184-96-250-76.hlrn.qwest.net. [184.96.250.76]) by smtp.gmail.com with ESMTPSA id gv23sm3300767pjb.17.2021.11.16.17.11.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 16 Nov 2021 17:11:05 -0800 (PST) Subject: Re: [PATCH] restore ancient -Waddress for weak symbols [PR33925] To: Jason Merrill , "Joseph S. Myers" Cc: gcc-patches References: <679cf10f-e16a-e318-0e82-820efb109d0f@gmail.com> <24cd9565-b127-6534-d98e-7482b3dc082f@gmail.com> <75dfaeb9-b6ee-6405-69e7-72ed32ff07ae@redhat.com> From: Martin Sebor Message-ID: Date: Tue, 16 Nov 2021 18:11:04 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <75dfaeb9-b6ee-6405-69e7-72ed32ff07ae@redhat.com> Content-Type: multipart/mixed; boundary="------------B26309A1354523352B5BBEF9" Content-Language: en-US X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Nov 2021 01:11:10 -0000 This is a multi-part message in MIME format. --------------B26309A1354523352B5BBEF9 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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. --------------B26309A1354523352B5BBEF9 Content-Type: text/x-patch; charset=UTF-8; name="gcc-33925.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-33925.diff" 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; } --------------B26309A1354523352B5BBEF9--