Spurious -Wnull-pointer-subtraction inside macros / unreachable code

[anarazel]

Hi,

Compiling postgres with clang 13 or newer triggers a host of -Wnull-pointer-subtraction warnings (discovered because the warning is part of -Wextra). To us it looks like the warnings are spurious.

Simplified reproducer (also at https://godbolt.org/z/ErfEc6Ezz):
compiling

#include <stddef.h>

#define relptr_store(base, rp, val) \
	 ((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))

typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;

void
problem_not_present(relptr *rp, char *base)
{
    struct foo *val = NULL;
    
    relptr_store(base, *rp, val);
}

void
problem_present(relptr *rp, char *base)
{
    relptr_store(base, *rp, NULL);
}

with just clang -Wnull-pointer-subtraction yields:

<source>:19:5: warning: performing pointer subtraction with a null pointer has undefined behavior [-Wnull-pointer-subtraction]
    relptr_store(base, *rp, NULL);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:4:60: note: expanded from macro 'relptr_store'
         ((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
                                                 ~~~~~~~~~~~~~~~~ ^
1 warning generated.

The way I read the standard legalese it's the evaluation that'd produce undefined behavior, but it never is evaluated here.

@jamieschmeiser, looks like this came in with your 9cb00b9 / https://reviews.llvm.org/D98798?

Regards,

Andres

[jamieschmeiser]

Hi @anarazel, the compiler can not always determine, at compile time, when an expression will be evaluated. The null pointer subtraction warning is not generated in cases where it can be determined that the expression would not be evaluated, as your example illustrates. The only way to ensure 100% accuracy would be to issue the warning at runtime, which, I'm sure you would agree, is undesirable. Consider the following variation of your testcase, involving a similar warning about undefined behaviour (with -Wextra):

#include <stddef.h>

struct foo{};

#define relptr_store(rp, val) \
	 ((rp).relptr_type = ((val) == NULL ? 0 : ((foo *) (val)) + (1)))

typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;

void
problem_not_present(relptr *rp, char *)
{
    struct foo *val = NULL;
    
    relptr_store(*rp, val);
}

void
problem_present(relptr *rp, char *)
{
    relptr_store(*rp, NULL);
}

tt.C:21:5: warning: performing pointer arithmetic on a null pointer has undefined behavior if the offset is nonzero [-Wnull-pointer-arithmetic]
    relptr_store(*rp, NULL);
    ^~~~~~~~~~~~~~~~~~~~~~~
tt.C:6:60: note: expanded from macro 'relptr_store'
         ((rp).relptr_type = ((val) == NULL ? 0 : ((foo *) (val)) + (1)))
                                                  ~~~~~~~~~~~~~~~ ^
1 warning generated.

The issue that you raise is, in fact, not about the warning per se, but rather about determining when an expression is known at compile time and a branch will or will not be evaluated.

[anarazel]

Hi @jamieschmeiser ,

Hi @anarazel, the compiler can not always determine, at compile time, when an expression will be evaluated.

Of course.

The null pointer subtraction warning is not generated in cases where it can be determined that the expression would not be evaluated, as your example illustrates.

I think my example rather illustrates that the warning isn't generated because Sema::CheckSubtractionOperands() doesn't even see that there's a NULL, because it's basically a purely syntactical check?

The issue that you raise is, in fact, not about the warning per se, but rather about determining when an expression is known at compile time and a branch will or will not be evaluated.

As far as I can tell there's not even attempt at detecting whether the code is unreachable or not. CheckSubtractionOperands() just uses Diag(), which emits unconditionally. Even if the expression is obviously unreachable it'll still be emitted.

Basically the example boils down to:

ptrdiff_t
problem_present2(char *base)
{
    ptrdiff_t diff = 0;
    if (0) 
        diff = base - (char *) NULL;
    return diff;
}

which still generates the warning.

I don't know the clang code at all, but shouldn't the warning at least be emitted with Sema::DiagRuntimeBehavior() to avoid this problem?

Regards,

Andres

[jamieschmeiser]

I checked in both the C and C++ standards and they both state that the behaviour is undefined, without differentiating whether the code in question is evaluated or not, so generating the warning on your sample is appropriate. This warning is produced in a way consistent with other similar warnings, which also appear to be issued regardless of whether or not the compiler can determine that the code is not evaluated. Consider the following alteration of your sample:

long
problem_present2(char *base)
{
    long diff = 0;
    if (0) 
      (void*)0 + 1;
    return diff;
}

A warning is issued, even though the code would not be evaluated. Both the warning here and in your sample are issued using similar code.

This warning is working as designed, is consistent with similar warnings, and is consistent with what the standards stipulate.
I do not think this issue is valid.

[anarazel]

I checked in both the C and C++ standards and they both state that the behaviour is undefined, without differentiating whether the code in question is evaluated or not, so generating the warning on your sample is appropriate.

Where do you read that in the standard? I'm by no means a standardese-interpretation expert, but to me C17's 6.5.6 9) doesn't talk about un-evaluated code triggering UB just because P and Q don't point into the same object. Wouldn't that result in a completely nonsensical world? You practically couldn't prevent UB with if()s anymore!

[anarazel]

This warning is produced in a way consistent with other similar warnings, which also appear to be issued regardless of whether or not the compiler can determine that the code is not evaluated.

Many of the other warnings are of a different "type". A type mismatch is a type mismatch whether the code is reachable or not, etc.

The comparable case of a null pointer dereference however, does use DiagRuntimeBehavior() (c.f. CheckForNullPointerDereference).

[jamieschmeiser]

I looked in the C++11 and C11 standards as I do not have ready access to the C17 standard. 6.5.6 [Additive Operators], p 9 states:

When two pointers are subtracted, both shall point to elements of the same array object,
or one past the last element of the array object; the result is the difference of the
subscripts of the two array elements. The size of the result is implementation-defined,
and its type (a signed integer type) is ptrdiff_t defined in the <stddef.h> header.
If the result is not representable in an object of that type, the behavior is undefined

Undefined behaviour is defined in 3.4.3 as something that the standard "imposes no requirements."

The null pointer is not pointing to an array object, nor one past the last element of an array object, so it is an error, although it is being flagged with a warning. Nowhere does it state anything about whether the expression is evaluated, so it is not allowed regardless of whether the code is evaluated or not. The result is not representable in an object of that type, hence the behaviour is undefined.

If one were to attempt to write code that completely prevents UB behaviour with pointer subtraction, one would have to ensure that both pointers were validly pointing at members of the same array, by comparing both to the start of the array and the known end (+1) of the array. Efficiency of such code would likely be a problem and it is not typically done. Staying within the realm of defined behaviour is done by ensuring that the pointers in question refer to the same array (or 1 past), typically through algorithmic design.

Subtracting pointers from 2 different arrays is likewise, undefined behaviour. One could not reasonably expect an optimizer to be able to maintain language semantics with such code. It may work, as unoptimized code or as optimized, or it may not. The standard "imposes no requirements" by stating that it is undefined behaviour.

The standard attempts to keep the world from being completely nonsensical by stating semantics for constructs so that they can reasonably be expected to work with conforming compilers/optimizers (ie, barring bugs), as long as the code being compiled also conforms to the standard. It also attempts to avoid forcing a conforming compiler to have certain aspects or features. Requiring that certain code constructs are only errors through the use of constant expression evaluation (such as you are advocating) is considered to be a quality of implementation issue, rather than an aspect of conformance to the standard. As such, one might consider suppressing the warning in an extended, lenient mode that allows non-conforming code. In this instance, the compiler does allow it by only issuing a warning rather than an error, which would prevent compilation of the code.

I would not consider a warning on a type mismatch nor a null pointer dereference a comparable situation. Detecting a potential null pointer dereference would be an example of where a compiler would attempt to use constant expression evaluation at compile time to warn of a potential (or imminent) error at runtime. It is an unary operator (different syntax), plus the dereference is a runtime error (it is semantically valid to use *((void*)0) but it is invalid at runtime). Since null pointer dereferences are devastating, compilers typically expend effort to detect this runtime error at compile time to aid developers in detecting such problems (a quality of implementation issue, not a conformance requirement). In the case of pointer subtraction, it is, in general, impossible to check. It is banned to allow reasonable optimization techniques to be performed on code involving pointers. The code is invalid but by diagnosing recognizable situations (ie, some null pointer situations), the compiler is enforcing the standard at a reasonable level and warning the user of code that may potentially break with other compilers, or indeed, other levels of the same compiler. Nothing is guaranteed.

Subtraction is an additive operator, of which there are 2: addition and subtraction. The warnings for addition are comparable (and illustrated in my examples) and the warning for subtraction is treated in the same way as the warnings for addition, so it is consistent with the comparable warnings.

[anarazel]

I looked in the C++11 and C11 standards as I do not have ready access to the C17 standard.

I don't think there's a meaningful difference between C11 and C17 here.

6.5.6 [Additive Operators], p 9 states:

When two pointers are subtracted, both shall point to elements of the same array object,
or one past the last element of the array object; the result is the difference of the
subscripts of the two array elements. The size of the result is implementation-defined,
and its type (a signed integer type) is ptrdiff_t defined in the <stddef.h> header.
If the result is not representable in an object of that type, the behavior is undefined

Undefined behaviour is defined in 3.4.3 as something that the standard "imposes no requirements."

That doesn't mean that it's UB if it just appears anywhere in the program though. C11 4.2:

If a ‘‘shall’’ or ‘‘shall not’’ requirement that appears outside of a constraint or runtime-
constraint is violated, the behavior is undefined. Undefined behavior is otherwise
indicated in this International Standard by the words ‘‘undefined behavior’’ or by the
omission of any explicit definition of behavior. There is no difference in emphasis among
these three; they all describe ‘‘behavior that is undefined’’.

Bolded the imo relevant part.

If one were to attempt to write code that completely prevents UB behaviour with pointer subtraction, one would have to ensure that both pointers were validly pointing at members of the same array, by comparing both to the start of the array and the known end (+1) of the array. Efficiency of such code would likely be a problem and it is not typically done. Staying within the realm of defined behaviour is done by ensuring that the pointers in question refer to the same array (or 1 past), typically through algorithmic design.

An if (0) is a form of "one would have to ensure that both pointers were validly pointing at members of the same array".

[jamieschmeiser]

3.8 states that a constraint is a restriction, either syntactic or semantic, by which the exposition of language elements is to be interpreted This is referring to constraints in the standard, not in the code. if(0) is not a constraint in the standard. The syntax rules, the semantic rules, etc, expressed in the standard are constraints.

[anarazel]

I'm doubtful about this interpretation for C, but the C++ language is clearer:

8.5.6 5)

When two pointers to elements of the same array object are subtracted, the type of the result is an
implementation-defined signed integral type; this type shall be the same type that is defined as std::ptrdiff_-
t in the header (21.2). If the expressions P and Q point to, respectively, elements x[i] and x[j]
of the same array object x, the expression P - Q has the value i − j; otherwise, the behavior is undefined.
[ Note: If the value i − j is not in the range of representable values of type std::ptrdiff_t, the behavior is
undefined. — end note

And UB is defined 4.1.1 5)

A conforming implementation executing a well-formed program shall produce the same observable behavior as
one of the possible executions of the corresponding instance of the abstract machine with the same program
and the same input. However, if any such execution contains an undefined operation, this document places
no requirement on the implementation executing that program with that input (not even with regard to
operations preceding the first undefined operation).

There's no execution containing the undefined behaviour in the case we're talking about?

Anyway, I gotta do some of my actual work. I just can tell you that from my POV the warning right now is triggering in cases that aren't helpful. We've now rewritten the code in postgres to to hide the NULL from clang sufficiently to prevent the warning (https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e07d4ddc55fdcf82082950b3eb0cd8f728284c9d), so I'm not personally affected anymore.

[efriedma-quic]

I agree this shouldn't trigger on unreachable code. And yes, DiagRuntimeBehavior exists for precisely this sort of scenario.

@jamieschmeiser I can't follow your argument. https://blog.regehr.org/archives/213 is a pretty good introduction to undefined behavior; maybe that helps?

[jamieschmeiser]

I checked with a local "language-lawyer" and he sided against me so I have assigned this to myself.

[anarazel]

I checked with a local "language-lawyer" and he sided against me so I have assigned this to myself.

Ping?

[jamieschmeiser]

Posted https://reviews.llvm.org/D126816 for review

[anarazel]

Thanks!