GCC should warn about 2^16 and 2^32 and 2^64

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90885

by Kliment • 7 years ago

108 133 7 years ago

gcc.gnu.org

ChrisRR• 7 years ago

For those who aren't C/C++ programmers, the issue here is people using ^ to mean power where it means XOR in C/C++.

fefe23• 7 years ago

Even if it meant power, it would make sense to warn here, because storing 2^16 in an unsigned short yields 0 :-)

cfstras• 7 years ago

these are signed, so you‘re getting -2^16... so many warnings to emit!

/edit: had that mixed up in my head. Interestingly, clang does warn about the overflow:

    main.c:6:17: warning: implicit conversion from 'int'
      to 'int16_t' (aka 'short') changes value from
      65536 to 0 [-Wconstant-conversion]
  int16_t s16 = 65536;

garmaine• 7 years ago

Yeah my own first reaction was “Oh, gcc doesn’t warn about overflow assignment? That’s a definite bug.”

I code in C/C++ too, although not exclusively. I facepalmed a few seconds later, but it goes to show the value of having this be a compiler warning.

quickthrower2• 7 years ago

I am not a C programmer but my opinion with any language is don’t try to catch mistakes like this in the compiler. Instead leave to configurable and competing linters for people who want to catch possible mistakes like this. Anyone caught by this mistake probably hasn’t tested their program adequately. Any mistake induced by the wrong constant number should be easy to weed out with unit testing.

scatters• 7 years ago

By the time you write a linter, you've done more than half of the work of a compiler, at which point you might as well just ship it as a compiler. It's a bit different for clang, which is modular and provides libraries that tool writers can use, but gcc is intentionally monolithic.

simion314• 7 years ago

Unit tests will not catch all issues, the person that writes the unit test will probably not see a corner case when implementing the code so it won't add the test for that corner case in the unit test either.

DownGoat• 7 years ago

Not including a linter in the earlier C compilers are one of the hughe mistakes of that area. Compilers absolutely should have linters, they are very valuable tools. Think about all the common security mistakes in your average C application that could have been avoided over the years with a linter.

I'm pretty sure Dennis Ritchie at some point himself said that not including a linter was a mistake, but I cannot find a quote of this with a quick search.

archi424• 7 years ago

By that logic you could get rid of a lot of warnings. TBH, I think getting good warnings is a major feature of modern compilers.

NikkiA3• 7 years ago

The kind of developers who are likely to make this mistake are probably not the kind of developers who will use advanced tools including linters or likely doing unit testing.

Some may even have released code that still has these time bombs waiting to occur.

pjmlp5• 7 years ago

lint was developed in 1979 (https://www.bell-labs.com/usr/dmr/www/chist.html) and we had to wait until clang for many developers start caring about actually using static analyzers.

Every external tool is a lost opportunity for quality enforcement.

aidenn02• 7 years ago

GNU's C linter is GCC warnings. For C, making a linter isn't a big deal, but for C++ writing a robust C++ parser is so hard that the linter will need to use the compiler's parser anyways.

dehrmann2• 7 years ago

And Java, Python, Javascript, Ruby. The list goes on. It's more of a general problem for non-programmers than specifically people who are new to C (unlike the j++ in python someone mentioned).

phaedrus• 7 years ago

I am a C++ programmer and yet I also failed to catch on that ^ in the ticket was xor not pow until I read the comments. All it took was being a bit sleepy.

A rather unfortunate operator choice on the part of C: open to confusion and precluded adding ^ as an exponentiation operator to the language.

AKrumbach• 7 years ago

Well, that isn't the only bug in the code, just the one which is being requested for prompting the warning. It's been about a decade since I've written significant C/C++ code, but something far more dangerous stood out to me:

Upon first reading, I'd assume they are seeing the maximum unsigned values, but their variables aren't declared as unsigned -- and C/C++ default to signed values. Secondly, representing the nth power of two actually takes n+1 binary digits, so e.g. 28 is a one, followed by 8 zeros -- overflowing any 8-bit variable, signed or unsigned! This is undefined behavior as-declared (signed values), but would be entered as a zero for explicitly declared unsigned variables.

Personally, if I were to be implementing this, I'd use bitwise operations to generate the correct values, and explicitly acknowledge in a comment something like "the following code knowingly breaks the abstraction of numeric values. It is likely not portable to other hardware, nor should you assume easy extension to other variable types. Das Quellcode ist nicht fur der gefingerpoken!"

NotPaidToPost6• 7 years ago

I don't think it would not be simple or a good idea to warn because these are perfectly valid expressions and it would lead to a flood of warnings if compilers started to display "Did you really mean that?" for an ever growing list of expressions (which is what it would become).

Better to leave this to code analysis tools.

Edit:

2^32 uses the ^ operator exactly as intended, and has perfectly legitimate uses, especially when dealing with registers of HW peripherals.

It's not the same as warning on "if (a=b)" as someone replied.

From a language perspective, 2^32 is exactly the same as 2+32.

rocqua4• 7 years ago

It's a warning not an error. Moreover, it is very hard to come up with a legitimate usecase of 2^32 as an expression. People who use it to set bit flags will use &, and for bit flags, octal or hex notation is probably better.

NotPaidToPost6• 7 years ago

It's a warning not an error.

Many build environments are set to treat all warnings as errors.

The point is that 2^32 is a perfectly compliant C expression that is neither misleading nor ambiguous, and that also won't create any variable overflow. It uses ^ exactly as intended. Why should the compiler complain? Why should I get a warning/error when following the spec to the letter?

pmikesell• 7 years ago

Yes, or 1 << 31, for example.

raxxorrax2• 7 years ago

I don't know if it is a good practice, but I often have statements and expressions that look awfully similar. Flipping bits does happen often enough.

Mostly written as y ^ (1 << x), which could easily resolve to these expressions. Mostly at runtime, sure, but there are exceptions. Especially if you like descriptive constants. I would expect it to be quite difficult to separate the "correct cases" without people starting to just suppress compiler warnings or trick it with writing the same stuff in different words (which might be better).

On the other hand, setting max_short to something like 18 is probably a really god prank in larger programming environments. The compiler would just ruin all the fun here.

blattimwind• 7 years ago

I don't think it would not be simple or a good idea to warn because these are perfectly valid expressions and it would lead to a flood of warnings if compilers started to display "Did you really mean that?" for an ever growing list of expressions (which is what it would become).

There are many technically valid expressions and statements that will still generate warnings in most compilers.

if(a=b) {} comes to mind first and foremost.

masklinn• 7 years ago

It's not the same as warning on "if (a=b)" as someone replied.

"if (a=b)" uses the "=" operator exactly as intended with perfectly legitimate uses. It's warned about because it's an error-prone construct, not because it's incorrect.

Seems to me 2^32 is also error-prone, if you wanted to combine flags you'd use 2|32, using xor here is weird.

NotPaidToPost6• 7 years ago

'If' expects a condition so finding an assignment may be a red flag even if an assignment has a return value in the spec, which makes the construct valid.

On the other hand ^ expects integers so 2^32 is exactly what is expected.

Most the replies I saw here try to second-guess or claim that ^ should be used in a specific way. Not so, ^ is just doing XOR of two integers.

Apparently, I am having an incorrect opinion, though, so I will self-censor and remain silent.

__david__2• 7 years ago

For the same reason that this:

    if (c)
        printf("Hello");
        exit(0);
    exit(1);

Produces a warning in gcc:

    test.c:7:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
         if (c)
         ^~
    test.c:9:9: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
             exit(0);
             ^~~~

"2^32" is of course just an xor, but it makes no sense (the "exclusive" part of it is not being exercised). If you really mean 2 bits being set in a constant then "2 | 32" is much clearer. If you're dead set on xor, "32^2" won't trigger the warning.

NotPaidToPost6• 7 years ago

It's not for you or the compiler to decide that 2^32 makes no sense. It is using ^ exactly as intended and is neither misleading nor ambiguous.

Stating that 32^2 should not trigger a warning while 2^32 should shows that this proposal has not been thought through.

It's not desirable or sensible to raise a warning on the premise that the expression might mean something else in another language, which is what this would do.

estebank2• 7 years ago

I'm intrigued at what your opinion is about the following Rust errors:

    error[E0308]: mismatched types
     --> src/main.rs:4:8
      |
    4 |     if x = y { println!("eq") }
      |        ^^^^^
      |        |
      |        expected bool, found ()
      |        help: try comparing for equality: `x == y`
      |
      = note: expected type `bool`
                 found type `()`

    error: `<` is interpreted as a start of generic arguments for `u32`, not a comparison
     --> src/main.rs:4:17
      |
    4 |     if i as u32 < 2 {
      |        -------- ^ --- interpreted as generic arguments
      |        |        |
      |        |        not interpreted as comparison
      |        help: try comparing the cast value: `(i as u32)`

ainar-g• 7 years ago

Dominik Honnef's staticcheck tool for the Go programming language will probably warn about these as well[1].

[1] https://github.com/dominikh/go-tools/issues/516.

TorKlingberg2• 7 years ago

It would be nice, but I can see it's not easy to come up with a warning heuristic that doesn't give a bunch of false positives. Sure, 2^16 is "obviously" wrong, but how about SOMETHING ^ XOR_KEY with #define SOMETHING and #define XOR_KEY 16?

DaiPlusPlus• 7 years ago

They discuss that in the thread. Consensus seems to be to warn when seeing “2 ^ (certain base-10 literals)”.

It’s uncommon to use base 10 when bit-twiddling with the XOR operator - so this makes sense.

Of course, it would be better if C-family languages didn’t use ^ for XOR in the first place.

mehrdadn2• 7 years ago

Do they not currently/do they plan to track macro expansion well enough to distinguish between macros and literals? I feel like they should be able to get the best of both worlds here.

simias2• 7 years ago

Of course, it would be better if C-family languages didn’t use ^ for XOR in the first place.

I think at the time they were created there simply wasn't an ambiguity in anybody's mind. POW is a relatively complex operation, I don't think anybody expected it as a builtin operator (much like SQRT and friends). Arguably so is DIV but I suppose that's too convenient not to have.

XOR on the other hand is something you really want to be a builtin operator in a low level, bit-twiddling language like C, so getting a dedicated operator makes sense. I'm not sure if an other sigil would've made more sense, after all ASCII lacks ⊕.

billpg2• 7 years ago

If we're using our time machines to fix C, can we make a special type that's identical to unsigned int but can use all the bit-wise operators, requiring an explicit conversion operators between regular int and bitwise int.

bitwise x = 0x1234; x |= 0x1000;

(0x indicates a bitwise value.)

Jyaif• 7 years ago

With clang you can write linters that get the AST of the code in input.

https://clang.llvm.org/extra/clang-tidy/

antirez3• 7 years ago

Terrible idea. If a programmer is so unfamiliar with C/C++ to believe ^ is pow(), she/he will create a lot of other problems regardless. Without to mention the kind of bugs created by such incorrect usage would be simple to spot most of the cases.

barrkel3• 7 years ago

I think you're being unrealistic about development in a team with mixed experience levels. These things slip through. We can stop them automatically, without needing to rely on a manually written test, or on being spotted in a code review. It's all upside from where I'm standing.

q3k2• 7 years ago

Experienced people make mistakes, and that's okay. I've made mistakes like this, or worse. I want my tools to help me write good code, not assume that I'm an infallible machine.

We need to kill the myth that good programmers don't need help from their tools.

raverbashing• 7 years ago

Incremental learning and guidance is important. If the compiler can point out something that's technically correct but in practice wrong (because 2^16 is the weirdest way of writing that value) it will help some people

OskarS3• 7 years ago

Wait... so your point is that since "bad programmers" do this, there shouldn't be a warning for it? That is among the silliest thing I ever heard. Especially since there's a trivial way to get rid of the warning: just write "18" instead of "2^16", if you REALLY intended the xor.

Also: it's incredibly naive to think that "bad programmers" are the only ones that would make a mistake like this. I'm very familiar with all C/C++ operators and know perfectly well that the caret represents xor, but I could easily imagine myself slipping and writing 2^16 instead of 1 << 16, because that's how the rest of the world writes exponentials.

Let he/she who has never written a typo and had the compiler save them cast the first stone.

mehrdadn2• 7 years ago

Wait... so your point is that since "bad programmers" do this, there shouldn't be a warning for it? That is among the silliest thing I ever heard.

In all honesty while I don't agree with this particular instance, the reasoning isn't ridiculous. If we assume it's true that it'd only affect bad programmers, then you probably wouldn't want to add it, since it'd increase the false positive risk and/or otherwise potentially get in the way of everyone else. (Kind of like how you wouldn't make cars start honking when they're turned on as a means of protecting against bad drivers, even thought that might well save lives.) Some tools just need to assume some base level of expertise to be effective.

antirez3• 7 years ago

Let's way that warnings are very important, they cause noise, so such noise should be deserved for instances that are really common mistakes among the population of programmers that have some clue. There are many errors like the one above that can be made by clueless programmers for which you can't emit a warning at all, like misusing postfixed/prefixed increment operators or alike.

simias2• 7 years ago

At first it was my knee-jerk reaction as well but it's bit dumb frankly. I write some python semi-regularly and even though I'm familiar with its syntax I commonly end up putting ; at the end of statements (which generally doesn't do anything for better or worse) and use && and || instead of 'and' and 'or', mainly out of muscle memory.

I can imagine a casual C coder could easily end up using ^ as "pow" for very much the same reason if they're used to it working that way in other languages. On top of that the proposed warning has extremely low chances of triggering on a false positive on actually legit code. I think it's a good proposal.

Without to mention the kind of bugs created by such incorrect usage would be simple to spot most of the cases.

That's not obvious to me at all. Bogus bitfield values could be rather tricky to track down, especially since they might work correctly "by chance" in simple cases (for instance even if the value of the constant is completely wrong, as long as it's != 0 you can set it with |= and test it with & and it'll appear to work mostly correctly).

dvhh• 7 years ago

My take is that for those who are trying to be as pedantic as using -Werror flags would have a lot of fun putting up pragmas to remove the the false positive.

bluefox• 7 years ago

I agree with you, but this thread is full of people thinking it's a good idea because they or their colleagues are prone to that kind of silliness. For people who write C or C++ every day it's stupid compiler second-guessing, and it can also create issues for generated code, but this thread shows that our tools may need adjustment to better deal with today's level of incompetence.

colejohnson662• 7 years ago

Why would you write 2 xor 16 in instead of just 18? And if it’s generated code, wrap it in a pragma to disable the warning

garethrees2• 7 years ago

This seems like a good idea to me, since there's evidence that programmers are making this mistake, and the proposal is to add a warning only for the expressions that are most likely to be mistakes, that is 2^{decimal integer literal} and maybe also 10^{decimal integer literal}. There are many constructs that are well-defined in C, but where it is helpful to have warnings: use of = in condition expressions, implicit fallthrough in switch statements, misleading indentation, unsafe multiple statement macros, and so on. Programming in C is hard enough that every bit of compiler support is valuable.

mrfredward2• 7 years ago

The power vs xor mistake is no doubt a common one, but I take issue with the 2^8, 2^16, 2^32 examples that got singled out because they are also common bitmasks. Should FLAG_BIT1 ^ FLAG_BIT3 really be a warning?

XOR may be rarely used, but 2^8 is not only one of the most common accidental XORs, it's also probably one of the more common legitimate uses of it.

tln• 7 years ago

Should FLAG_BIT1 ^ FLAG_BIT3 really be a warning?

Nope, the warning would be for literal numbers in the source. IMO anyone who doesn't define constants for the flag bits deserves the warning.

fanf2• 7 years ago

Flag bits should be combined with | not ^

nialv72• 7 years ago

This is an example of why you shouldn't use blanket warning options like -Wall in combination with -Werror. Otherwise your program might fail to compile just because the dev randomly thought of some new things to warn about.

archi424• 7 years ago

We use -Wall and -Wextra, and fixed basically all our warnings. Now everyone writes virtually warning-free code (that is, any warning is fixed before committing). And I don't feel limited in what I can do.

We could roll -Werror for most packages (I think we have a few packages, e.g. 3rd party libs, which emit warnings), but only use error=format and error=format-extra-args.

To be fair, we disable these warnings (-Wno-...): unknown-pragmas, long-long, register, unused-command-line-argument.

seba_dos1• 7 years ago

I just keep my codebase warning free and check it in CI. No need for -Werror on package level, users don't need to deal with random breakage from a false-positive warning.

adrianN• 7 years ago

You should always thoroughly check whether your program still works after upgrading the compiler. Adding a -Wno-bad-exponentation to your makefiles shouldn't be that difficult.

nialv72• 7 years ago

If you are the only one using your program, then that's probably fine. I cannot count how many times I have to patch other people's makefile so their code won't fail to compile because of some -Werror introduced in a newer version of the compiler.

TorKlingberg2• 7 years ago

-Wall, despite its name, isn't actually all warnings. Compiler devs only rarely put new warnings into -Wall.

floatboth• 7 years ago

They do it often enough that OS package maintainers deal with build failures caused by Werror quite a lot.

And forget new versions of compilers, how many things are only tested with GCC and fail to build with Werror with clang because of different warnings in Wall?

pm215• 7 years ago

I like the intermediate position: enable -Werror for development builds (which for QEMU means "builds from head-of-git"), leave things as just warnings for (source) releases, and give the user an escape hatch (eg a configure --disable-werror option) in case they need it. Enforcing warning-free compiles for the average developer is really useful for maintaining a minimum quality bar, so it's good to find a way to keep that without annoying people who might be compiling from source but are essentially 'end-users' of it.

marcosdumay2• 7 years ago

You don't need -Werror for enforcing warning-free compilation.

oytis2• 7 years ago

What if I try to make a pipe with '|' or redirect the result with '>'? Should there be a warning for that?

I find it amusing that people really do that though.

sambe• 7 years ago

Some people switch programming languages often. Exponentiation operators are common, both caret and double star are used. It does not seem amusing (surprising?) to me.

On the other hand, piping and redirection are relatively uncommon outside of the shell. There is certainly a clearer mental distinction for me between "I am in shell" and "I am not in shell" than "I should double star".

oytis2• 7 years ago

C doesn't have double star either (in fact double star means double dereferencing).

YMMV or course, but distinction between a low-level language like C, and a higher-level one that can support exponentiation, matrix multiplication, thread spawning etc. as a first-class language construct is as clear to me as the difference between shell and non-shell.

maxlybbert• 7 years ago

The warning amounts to “only someone who knows what they’re doing would write XXX, and we believe you don’t know what you’re doing.”

deathanatos• 7 years ago

I recently found several instances of hand-crafted for loops in our Python code with:

  j = 0
  while condition():
    ++j

In Python, there is no "increment" operator, so `++j` is parsed as two unary plus operators: it's a no-op.

dagw2• 7 years ago

I recently found the following bug in some python code at work

  x = 12 //10

What they where apparently trying to do was to change the value of x while commenting out the old value for reference. Except in python // is integer division and not comments so x was set to 1 instead of 12.

alexeiz• 7 years ago

This is silly. One of my personal stupid mistakes in Python is forgetting to delete a trailing comma. The resulting code is still valid, because a trailing comma produces a tuple.

    a = b,

guitarbill• 7 years ago

the bugbear plugin for flake8 can warn about this: https://pypi.org/project/flake8-bugbear/

cryptonector• 7 years ago

From the HN submission title it wasn't obvious that the issue is people misunderstanding that the ^ operator isn't the exponential operator but the XOR operator. Clicking through made it clear.

Warning about constant subexpressions where the LHS and RHS of ^ are constant literals makes a lot of sense to me, but if the LHS is a constant expression but not a constant literal, then not so much. Also, warning about this will cause some false positives where macro expansions are involved -- probably not a good thing.

typopl• 7 years ago

My perl script to scan C/C++ source code for common programming typos can check for this particular case since 1999.

see https://www.cpan.org/authors/id/T/TY/TYPO/typo-2.46.pl

  // START: Mon Jun 17 14:49:53 2019
  C:\src\test\test.c (1): using ^ on a number 37: short#maxshort=2^16;
  C:\src\test\test.c (2): using ^ on a number 37: int#maxint=2^32;
  C:\src\test\test.c (3): using ^ on a number 37: long#maxlong=2^64;

Link to the 1999 Perl Conference paper for the script:

https://web.archive.org/web/20090729104734/http://www.geocit...

kazinator2• 7 years ago

GCC should understand the expression grammars of no fewer than a dozen other languages and warn you when something could syntactically be an expression from another language, but with a different meaning.

GCC should also change your diaper, burp you, and issue random diagnostics like

  foo.c:32: warning: did you test this yet?

kazinator2• 7 years ago

GCC has gotten too bloated. It could benefit from compile time CONFIG_ options to remove all the fluff, such as superfluous diagnostics.

If for whatever reason I have to include GCC in an embedded this sort of "misleading indentation" and "suggest parentheses" nonsense just wastes precious flash space.

GCC should warn about 2^16 and 2^32 and 2^64

Related Stories