Bug 102967 - confusing location in -Waddress for a subexpression of a ternary expression
Summary: confusing location in -Waddress for a subexpression of a ternary expression
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
: 105628 110402 114609 (view as bug list)
Depends on:
Blocks: Waddress
  Show dependency treegraph
 
Reported: 2021-10-27 18:14 UTC by Andrew Cooper
Modified: 2024-04-05 18:04 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-10-27 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Cooper 2021-10-27 18:14:28 UTC
Hello,

I have been compiling Xen with gcc/master, and found what appears to be a regression from GCC 11.

https://godbolt.org/z/qa61fs1hK is a simplified repro.

The address comparison seems to be incorrectly applied to only of the two possible options.  The expression as a whole is most certainly not unconditionally true.
Comment 1 Martin Sebor 2021-10-27 18:55:32 UTC
The warning is intended: it points out that the second operand of the conditional expression is necessarily true:

  if ( !(pa ? &pa->c : NULL) )
              ^^^^^^

There's no point in testing the address of a member for equality to null because  the member of no object can reside at that address.  The above can be simplified to

  if (!pa)

Below are a couple of small examples to illustrate.

I confirm this as a report of the underlining being confusing.  In the message on Godbolt:

   20 |     if ( !(pa ? &pa->c : NULL) ) // Macro expanded, just to check.  Still wrong.
      |          ^

and in the first message below it could stand to be improved to point to the second operand rather than the first (as I did above).

I also note that the C front end diagnoses both expressions as expected but the C++ front end only the latter one.  That seems like an omission to me that should be fixed.

$ cat a.c && gcc -S -Wall a.cstruct A { int i; };

int f (struct A *p)
{
  if (p ? &p->i : 0)   // -Waddress
    return 0;
  return 1;
}

int g (struct A *p)
{
  if (&p->i)           // -Waddress
    return 0;
  return 1;
}

a.c: In function ‘f’:
a.c:5:7: warning: the comparison will always evaluate as ‘true’ for the address of ‘i’ will never be NULL [-Waddress]
    5 |   if (p ? &p->i : 0)   // -Waddress
      |       ^
a.c:1:16: note: ‘i’ declared here
    1 | struct A { int i; };
      |                ^
a.c: In function ‘g’:
a.c:12:7: warning: the comparison will always evaluate as ‘true’ for the address of ‘i’ will never be NULL [-Waddress]
   12 |   if (&p->i)           // -Waddress
      |       ^
a.c:1:16: note: ‘i’ declared here
    1 | struct A { int i; };
      |                ^

When reporting bugs, please be sure to include the full test case and its output as requested at https://gcc.gnu.org/bugs/#need.  Links to external sites are not a substitute.  They might stop working, or the compiler used there might become out of date, leaving us with insufficient detail to analyze the report.
Comment 2 Andrew Cooper 2021-10-27 19:14:41 UTC
(In reply to Martin Sebor from comment #1)
> The warning is intended: it points out that the second operand of the
> conditional expression is necessarily true:
> 
>   if ( !(pa ? &pa->c : NULL) )
>               ^^^^^^
> 
> There's no point in testing the address of a member for equality to null
> because  the member of no object can reside at that address.  The above can
> be simplified to
> 
>   if (!pa)

Hmm, true.  I suppose I got hung up on the statement made by the diagnostic, rather than the implication that a simplification could be made.

Moving the underlining would certainly help.

> When reporting bugs, please be sure to include the full test case and its
> output as requested at https://gcc.gnu.org/bugs/#need.

My apologies.  I'll try to be better next time.
Comment 3 jbeulich 2021-11-04 12:12:20 UTC
(In reply to Martin Sebor from comment #1)
> The warning is intended: it points out that the second operand of the
> conditional expression is necessarily true:
> 
>   if ( !(pa ? &pa->c : NULL) )
>               ^^^^^^
> 
> There's no point in testing the address of a member for equality to null
> because  the member of no object can reside at that address.

Except that if pa is NULL and c is the first member of typeof(*pa), then &pa->c is also NULL, isn't it?

>  The above can
> be simplified to
> 
>   if (!pa)

If there was no macro expansion involved in the former case, I'd agree with the option of simplifying the expression. But the use of the macro expanding to this construct may be intended, perhaps at least for the code to be self-documenting. The macro itself, in turn, may be (and in our case is) also used in contexts where the produced pointer actually gets used as such, not for boolean NULL / non-NULL checks.

In any event I don't see how it matters here that, as you say, "the second operand of the conditional expression is necessarily true". Especially considering the macro aspect, changing to "pa ? 1 : NULL" or (to make the construct type-correct) "pa ? 1 : 0" isn't an option, yet that's what you appear to be suggesting (as intermediate step to arrive at simply "pa").

Imo the compiler wrongly connects the use of &pa->c in the conditional expression to the boolean context of the enclosing if(), even irrespective of the use of !() around the conditional operator. IOW

    if ( pa ? &pa->c : NULL )

should also go through without any diagnostic (at least as long as the compiler doesn't also take into account whether the expression is the result of macro expansion). Otherwise all you do is force people to write less legible code, e.g. the macro becoming

#define a_to_c(pa) ({ \
    type_of_c *pc_ = (pa) ? &(pa)->c : NULL; \
    pc_; \
})

based on Andrew's observation that breaking out the expression doesn't result in any warning (didn't check whether that in practice extends to the case above).
Comment 4 Martin Sebor 2021-11-04 15:35:43 UTC
The expression pa->c is only valid if pa points to a valid object.
Comment 5 jbeulich 2021-11-04 15:51:27 UTC
(In reply to Martin Sebor from comment #4)
> The expression pa->c is only valid if pa points to a valid object.

Well, yes, you may not deref pa if it's NULL, i.e. I agree for pa->c. But is &pa->c actually a deref?
Comment 6 Andreas Schwab 2021-11-04 16:35:20 UTC
&*E is allowed for E == NULL, but I don't think this can be generalized to &E->m.
Comment 7 Martin Sebor 2021-11-05 00:26:00 UTC
Both for the purposes of the warning (which can be more restrictive than what the language considers valid), and in the C language, the semantics of the -> expression depend on the first operand designating an object.  In C they're defined like so:

  A postfix expression followed by the -> operator and an identifier designates a member of a structure or union object.  The value is that of the named member of the object to which the first expression points, and is an lvalue. 106)

Footnote 106) If &E is a valid pointer expression (where & is the "address-of" operator, which generates a pointer to its operand), the expression (&E)->MOS is the same as E.MOS .
Comment 8 jbeulich 2022-05-27 09:36:22 UTC
(In reply to Martin Sebor from comment #7)
> Both for the purposes of the warning (which can be more restrictive than
> what the language considers valid), and in the C language, the semantics of
> the -> expression depend on the first operand designating an object.  In C
> they're defined like so:
> 
>   A postfix expression followed by the -> operator and an identifier
> designates a member of a structure or union object.  The value is that of
> the named member of the object to which the first expression points, and is
> an lvalue. 106)

While gcc 12 has meanwhile been released, it's still not clear how we could at least work around this (mis-)diagnostic, _without_ simplifying the conditional expression (which, as said, actually is the result of macro expansion, and the macro should not be open-coded [and then simplified] here).

It's also not just an issue with the underlining in the diagnostic - the overall expression !() as well as the conditional expression on its own isn't "always true" or "always false". It's only the 2nd operand of the ternary expression which is, but that's not on its own used in boolean context. Otherwise why would

	if(ps ? (void*)1 : (void*)0)

not result in a similar warning? And note that while

	if(ps ? &ps[10] : (void*)0)

does,

	if(ps ? &(&ps[10])[-10] : (void*)0)

again doesn't, while from &ps[10] being non-NULL (pointing at or one past a valid object) it imo ought to follow that &(&ps[10])[-10] is also necessarily non-NULL.

Note further that if the two array indexes don't sum up to 0, the offset reported in the diagnostic also looks to be wrong - it appears to lack division by sizeof(*ps).
Comment 9 Aidan MacDonald 2022-09-19 19:57:28 UTC
I am also hitting this with GCC 12.2. One more detail to add is that the info manual says the -Waddress warning should be suppressed if it's a result of a macro expansion. This does indeed happen when the macro is the only thing in the conditional, but not when the macro is part of a larger expression.

Example (https://godbolt.org/z/YEY4Yzdnf):

    #include <stddef.h>

    #define MACRO(buf, off) (off < 0 ? NULL : (void*)&buf[off])

    void func(char *buf, long off)
    {
        if (off < 0 ? NULL : (void*)&buf[off]) { } // warning (correct)
        if (!(off < 0 ? NULL : (void*)&buf[off])) { } // warning (correct)
        if (MACRO(buf, off)) { } // no warning (correct)
        if (!MACRO(buf, off)) { } // gives a false positive -Waddress
    }
Comment 10 Andrew Pinski 2024-03-11 12:34:36 UTC
*** Bug 105628 has been marked as a duplicate of this bug. ***
Comment 11 Andrew Pinski 2024-04-05 18:03:43 UTC
*** Bug 114609 has been marked as a duplicate of this bug. ***
Comment 12 Andrew Pinski 2024-04-05 18:04:15 UTC
*** Bug 110402 has been marked as a duplicate of this bug. ***