Feature request - allow boolean operations of undefined cpp symbols

Feature request - allow boolean operations of undefined cpp symbols

[Pavel Roskin]

Hello!

I think sparse should distinguish between safe and unsafe preprocessor
operations on undefined symbols.

For instance, "#if SYMBOL" has a very specific meaning, namely in
enables the subsequent code is SYMBOL is defined and not equal 0.
Implementing this in a "sparse friendly way" would be "#if
defined(SYMBOL) && SYMBOL", which is longer and more prone to errors
(the same symbol is used once, and nobody will warn in the first
instance is mistyped).

Some projects use configuration files with entries like "#define FEATURE
1".  Some users will replace "1" with "0", some users will comment it
out.  Supporting both requires "#if FEATURE", as opposed to "#ifdef
FEATURE", or somebody will be bitterly surprised that his or her edits
had no effect.

On the other hand, "#if FOO > BAR" is highly suspicious if FOO is
undefined.  Maybe sparse could allow boolean (and only boolean)
operations on undefined cpp symbols?

Unlike Linux kernel, the cases of "#if SYMBOL" with undefined SYMBOL
exist in glibc and other system libraries.  Relaxing sparse check would
make sparse more useful for checking other software.

-- 
Regards,
Pavel Roskin

Re: Feature request - allow boolean operations of undefined cpp symbols

[Linus Torvalds]

On Fri, 2 Feb 2007, Pavel Roskin wrote:
> 
> I think sparse should distinguish between safe and unsafe preprocessor
> operations on undefined symbols.

It does.

Try this:

	#define NECESSARY 1

	#if NECESSARY || UNNECESSARY
	#endif

and notice how sparse does NOT warn about UNNECESSARY not being defined. 
Because it doesn't matter.

> For instance, "#if SYMBOL" has a very specific meaning

No.

	#if SYMBOL

has a very specific *problem* - it very possibly is a typo.

So this is a warning I absolutely *want* for the kernel. If some other 
projects don't want it, fine, but it should be on by default as a warnign 
for potentially dangerous use of preprocessor symbols.

		Linus

Re: Feature request - allow boolean operations of undefined cpp symbols

[Christopher Li]

On Fri, Feb 02, 2007 at 10:25:12AM -0800, Linus Torvalds wrote:
> No.
> 
> 	#if SYMBOL
> 
> has a very specific *problem* - it very possibly is a typo.
> 
> So this is a warning I absolutely *want* for the kernel. If some other 
> projects don't want it, fine, but it should be on by default as a warnign 
> for potentially dangerous use of preprocessor symbols.
> 
I did some trivial test:

#if SYMBOL_NOT_DEFINED
#warning "defined"
#else
#warning "not defined"
#endif

On sparse git tip, or sparse 0.2. Here is what I get:

./sparse /tmp/a.c 
/tmp/a.c:5:2: warning: "not defined"

That is what Pavel wants. So Linus should complain instead.

I am confused.

Chris

Re: Feature request - allow boolean operations of undefined cpp symbols

[Christopher Li]

On Fri, Feb 02, 2007 at 05:30:37PM -0500, Pavel Roskin wrote:
> 
> You need -Wundef or -Wall to check for undefined preprocessor symbols.
> 
> $ sparse -Wundef test.c 
> test.c:1:5: warning: undefined preprocessor identifier
> 'SYMBOL_NOT_DEFINED'
> test.c:4:2: warning: "not defined"

Ha, I see. So do you still want some thing more than -Wno-undef?

Chris

Re: Feature request - allow boolean operations of undefined cpp symbols

[Pavel Roskin]

On Fri, 2007-02-02 at 13:56 -0800, Christopher Li wrote:

> I did some trivial test:
> 
> #if SYMBOL_NOT_DEFINED
> #warning "defined"
> #else
> #warning "not defined"
> #endif
> 
> On sparse git tip, or sparse 0.2. Here is what I get:
> 
> ./sparse /tmp/a.c 
> /tmp/a.c:5:2: warning: "not defined"
> 
> That is what Pavel wants. So Linus should complain instead.
> 
> I am confused.

You need -Wundef or -Wall to check for undefined preprocessor symbols.

$ sparse -Wundef test.c 
test.c:1:5: warning: undefined preprocessor identifier
'SYMBOL_NOT_DEFINED'
test.c:4:2: warning: "not defined"

-- 
Regards,
Pavel Roskin

Re: Feature request - allow boolean operations of undefined cpp symbols

[Linus Torvalds]

On Fri, 2 Feb 2007, Christopher Li wrote:
> 
> On sparse git tip, or sparse 0.2. Here is what I get:
> 
> ./sparse /tmp/a.c 

Try again, but with "-Wall".

		Linus

Re: Feature request - allow boolean operations of undefined cpp symbols

[Pavel Roskin]

On Fri, 2007-02-02 at 14:10 -0800, Christopher Li wrote:
> On Fri, Feb 02, 2007 at 05:30:37PM -0500, Pavel Roskin wrote:
> > 
> > You need -Wundef or -Wall to check for undefined preprocessor symbols.
> > 
> > $ sparse -Wundef test.c 
> > test.c:1:5: warning: undefined preprocessor identifier
> > 'SYMBOL_NOT_DEFINED'
> > test.c:4:2: warning: "not defined"
> 
> Ha, I see. So do you still want some thing more than -Wno-undef?

My intention was that we always allow boolean operations of undefined
cpp symbols, regardless of the flags.

Since Linus wants the existing strictness of -Wundef for the kernel, and
the kernel is sparse's "number one customer", I'll need to think of
something better to accommodate both the kernel and the userspace.

-- 
Regards,
Pavel Roskin

Re: Feature request - allow boolean operations of undefined cpp symbols

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

Linus Torvalds wrote:
> On Fri, 2 Feb 2007, Pavel Roskin wrote:
>> I think sparse should distinguish between safe and unsafe preprocessor
>> operations on undefined symbols.
[...]
>> For instance, "#if SYMBOL" has a very specific meaning
> 
> No.
> 
> 	#if SYMBOL
> 
> has a very specific *problem* - it very possibly is a typo.
> 
> So this is a warning I absolutely *want* for the kernel. If some other 
> projects don't want it, fine, but it should be on by default as a warnign 
> for potentially dangerous use of preprocessor symbols.

I looked this behavior up in the C99 standard, and found the following text in
section 6.10.1:
> Prior to evaluation, macro invocations in the list of preprocessing tokens
> that will become the controlling constant expression are replaced (except
> for those macro names modified by the defined unary operator), just as in
> normal text. If the token defined is generated as a result of this
> replacement process or use of the defined unary operator does not match one
> of the two specified forms prior to macro replacement, the behavior is
> undefined. After all replacements due to macro expansion and the defined
> unary operator have been performed, all remaining identifiers are replaced
> with the pp-number 0, and then each preprocessing token is converted into a
> token. The resulting tokens compose the controlling constant expression
> which is evaluated according to the rules of 6.6.

This states that we must substitute 0 for any undefined preprocessor symbol
in a #if or #elif condition, no matter what kind of expression they show up
in.  I confirmed this behavior via both GCC and Sparse; in an #if, "-1 <
SOMESYMBOL" evaluates true, and "1 < SOMESYMBOL" evaluates false.  Sparse
follows this spec precisely with respect to actual preprocessor behavior; it
simply has the ability to warn.

GCC has an equivalent option, also named -Wundef, that also generates a
warning.  Like Sparse, GCC does not issue this warning by default; GCC does
not include it in -Wall either.  For both Sparse and GCC, you have to turn it
on the warning for it to occur.

All of this makes me disinclined to turn -Wundef on by default.  However, I
also see no reason to change the current behavior of -Wundef.  GCC will also
give you a warning if you give -Wundef.  Just don't pass -Wundef if you have
valid conditionals in your project that generate warnings about undefined
preprocessor symbols.

However, Pavel, if you feel you could make part of -Wundef suitable to join
the default set of warnings, feel free.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: Feature request - allow boolean operations of undefined cpp symbols

[Pavel Roskin]

On Fri, 2007-04-20 at 03:02 -0700, Josh Triplett wrote:

> This states that we must substitute 0 for any undefined preprocessor symbol
> in a #if or #elif condition, no matter what kind of expression they show up
> in.

My point was that it's more likely to be a user error in case of
non-trivial expressions.  But arguing with Linus about probabilities of
user errors and other fuzzy matters is not something I'm prepared to.

> However, Pavel, if you feel you could make part of -Wundef suitable to join
> the default set of warnings, feel free.

I really don't care about this part.  I actually think that sparse
should augment gcc capabilities rather than duplicate them.  If we
cannot do better than gcc, we can just stop bothering.

-- 
Regards,
Pavel Roskin