Hacker News new | past | comments | ask | show | jobs | submit login
C, C++, x86/x64 assembly: The case of forgotten return (yurichev.com)
97 points by ingve on Dec 14, 2018 | hide | past | favorite | 49 comments



"-Wall -Wextra -Werror" are you allies :)


Please, no, don't use -Werror with -Wall or -Wextra. When you write your code and compile with -Werror on your old version of GCC (either because you're behind on GCC releases, or because you wrote the code in the past), and I want to compile it with my new version which has added some warnings, I don't want to have to mess around in your build configuration and remove -Werror.

Instead, do any of the following:

* Compile normally without -Werror, but have a test suite which compiles all your tests with -Werror. Make sure to only accept patches which don't break the test suite (probably using some CI system). If you don't want actual tests, a "test suite" which just makes sure `make clean && make CFLAGS=-Werror` doesn't fail would suffice.

* Use -Werror for regular builds, but enable only a fixed set of warnings, not -Wall or -Werror.

* Make your build system record warnings, and complain about them for every single build, not just the builds which happen to recompile the files which trigger warnings. This doesn't _guarantee_ that your project is warning-free like the other two do, but I imagine a lot of the reason projects end up with warnings is that people just see the warning once, and then never again until the file has to be recompiled.

I can't count the number of times I have wanted to use some project, but then had to dig through the project's Makefile or autotools m4/shell soup or gn files or meson files or cmake files or ad-hoc compile script or some other build configuration, just because my compiler is newer than the author's.


> Please, no, don't use ... when distributing your code

FTFY. It's fine to use them for development, but a major pain in the ass if you are distributing your code for others to compile (looking at you, google).


Yeah, I don't care how people or organizations actually write their code, I just want to download the code they distribute (through git or tarballs) and run the commands listed in a readme without the build failing because GCC thinks there's an unnecessary number of parentheses around an expression.

The three alternatives were meant as somthing easy one could do instead of blindly using using -Werror to achieve the same advantages, but there are of course more ways to do it.

I have also had the fortune of using Goole code. I have been meaning to write a blog post about the issues one encounters when using a Goole library without being a part of Google.


I actually don't even like -Werror for development. When I'm developing/testing/debugging, I frequently want to run partially implemented functions (a good example of this is repeatedly commenting out blocks of code to bisect a problem). Trying to deal with warnings about unused variables/parameters is extremely cumbersome.


I want to opt out of bad warnings-as-errors, rather than failing to opt into good ones. -Wall -Wextra -pedantic -Werror for me - followed by a bunch of -Wno-xyz like -Wno-unused-variable.

You could also use -Wno-error=unused-variable, but I find warnings as warnings almost completely pointless outside of extremely niche circumstances. I will lose them in a sea of other errors when a template goes awry, and after fixing that and building again the warning will disappear because the TU with said warning didn't need rebuilding.


I agree. The main place I find -Werror to be valuable is on the CI server, where you're checking that committed code compiles cleanly on your target platforms.


I like having a makefile variable such that you can do "make NO_WERROR=1"


Absolutely not the fault of Werror. Werror is fine and a great idea. Your complaint here is crappy build systems where you can't easily adjust flags. Yeah that's garbage and so normal we sometimes accept it without thinking how utterly rubbish it is. But it's sure not Werror at fault of you update things and have trouble working out how to do something basic and trivial with the build system. Werror forever. Don't do crappy build systems so you can do sensible things like enabling Werror.


I'm not sorry, but using a build system which makes it easy to disable -Werror doesn't help. It's still somehing everyone who wants to use your program has to figure out how to do, and if you're actually using an obscure build system (which it sounds like you're suggesting), then that's even more annoying. If everyone on the planet used one build system which had a singular way to disable -Werror you'd have a point, but that's not exactly going to happen.

It's not that it's usually _hard_ to figure out how to disable -Werror. In a correctly written Makefile, it's literally just `make CFLAGS=-Wno-error`. It's about having to figure it out at all. Don't make life annoying for every one of your users just because you're too lazy to set up your project such that builds used while developing (such as debug builds) use -Werror, while regular builds don't.

If you absolutely need every build to use -Werror, then please, just include your build system's version of `CFLAGS=-Wno-error` in your readme's compile instructions (even when using make; `CFLAGS=-Wno-error` works for _correctly written_ makefiles, which most makefiles are not.)


You've got build system Stockholm syndrome! :-) Changing a flag because you've changed compiler (or any other reason) should take no thought in the 5 seconds it takes. Any flag change. The end.


But... this isn't about my own code using my preferred build system. Of course you can quickly change any flag if you know the build system works and hiw your project uses it. It will not take 5 seconds with no though to figure out how a build system you have never used before lets you change build flags. Besides, even if it did take just 5 seconds and no thought, I wouldn't know it's necessary before I've tried to compile, maybe noticed that it will take a while because it's a big project, gone away and done something else for a bit, and returned to a failed build because one file a couple of minutes in had an unnecessary set of parentheses somewhere.

I absolutely don't understand what stockholm syndrome has to do with anything. I haven't advocated for any particular build system.


If you can change build systems flags easily, without thought, very quickly. Do you still think Werror is a bad idea?

Assuming your answer to that is the same as mine. If you always have a build system where you can change flags easily, without thought very quickly, you do use Werror. We frequently (always?) don't have such a build system and we're used to that. We're prisoners of it. It sucks. C build systems are utterly terrible. We're captured by them. Whether it's autotools, cmake, scons, a bajillion make files used directly or whatever. I think acknowledging it is reasonable. I think claiming they are other than terrible is stockholm syndrome. (Note that they may not be capable of being improved, there may be nothing better, etc etc. - I make no claims about it).

Not using a flag because it makes life hard when the compiler is upgraded and the flag needs to be changed is a build system issue. Really.


> Please, no, don't use -Werror with -Wall or -Wextra. When you write your code and compile with -Werror on your old version of GCC (either because you're behind on GCC releases, or because you wrote the code in the past), and I want to compile it with my new version which has added some warnings, I don't want to have to mess around in your build configuration and remove -Werror.

100% disagree. If compilers add new warnings it's because they have new insights in your code, and old invalid but "working" code may not work at all in the newer version and eat your hard disk. So instead of risking your data, just accept that it won't work with this newer version of the language.


A new compiler version isn't a new version of the language. Most warnings are about completely valid code which nonetheless should be avoided. Code which was correct in an earlier version if GCC continues to be correct in new versions of GCC.

Unused variables, using `if (somevar = othervar)` instead of `if ((somevar = othervar))`, code which uses operators without parentheses where GCC has decided the operator precedence is unexpected and should have extra parentheses to be clearer, a switch which doesn't cover all the values of an enum, unnecessary parentheses in a variable declaration. Those are all warnings which absolutely don't mean that the code is broken. The author should absolutely fix new warnings like that, but that shouldn't be the responsibility of someone who just wants to use your open source project.


I agree about open source projects, then compiler options should not be so strict. But during development it might be useful; I use -Werror before releasing piece of code, just not to add any new warnings.

BTW, a few years ago we had a nasty bug (feature?), that might have been prevented when converted into error, but... "because we always have very large build logs I didn't notice the new warning." http://0x80.pl/notesen/2015-03-22-compiler-warnings.html


You can qualify -Werror so it only applies to missing return statements:

-Wall -Wextra -Werror=return-type


-Weverything if using clang


It's too noisy just by itself. First time I tried it, it complained about padding added to structures. When I packed them, it complained about packed structures.


Manually pad them, and it'll complain about unused fields ;)

I've actually had some pieces of code where it was super useful to enable that padding warning as an error. Some complicated C++ types that needed to have identical memory layouts across compilers yet often weren't because of some really nasty shenanigans...

-Weverything is for people who really want to explicitly opt out of every warning they don't like.


Not recommended. Particularly there are warnings for C++ now that are "mutually unsatisfy-able."


it's much better to start from -Weverything and add -Wno-whatever (e.g. -Wno-c++11-compat for instance which complains about C++11 features) than start from -Wall -Wextra etc...


Any examples?


It's a little bit awkward to watch a new generation rediscover things which used to be common knowledge. It's a good thing, though; this sort of thing shouldn't be as common a practice as it once was.


My mind is blown that g++ (7.3.0) doesn't warn on this.


It seems like GCC 8 warns me by default, so someone must have realized that this was stupid:

  $ pbpaste | g++-8 -x c++ -
  <stdin>: In function 'color* create_color(int, int, int)':
  <stdin>:19:1: warning: no return statement in function returning non-void [-Wreturn-type]


That is the correct behavior for c.

But you mentioned g++: not sure when this became invalid in c++ but for me g++ -std=17 warns by default (though we compile everything with maximum possible warnings already)


From an ISO C conformance viewpoint, it is neither "correct" nor "incorrect" to fail to generate a diagnostic that is not required by ISO C.

In the ISO C++ language, I vaguely seem to recall ("I could have sworn that") that failing to return a value from a function is a diagnosable semantic rule violation. And that it has been for a very long time.

So I fully sympathize with the "mind is blown" comment in grandparent.

Hey, g++ -pedantic catches some important semicolon trespasses, though:

  noreturn.cc:19:2: warning: extra ‘;’ [-pedantic]
  noreturn.cc:25:2: warning: extra ‘;’ [-pedantic]
:)


> Non-optimizing GCC 5.4 silently compiles this with no warnings

GCC 5.4 is more than two years old; the current version is 8.2¹.

1. https://www.gnu.org/software/gcc/releases.html


But when you are an enterprise too afraid to upgrade your version of RHEL then GCC 5.4 <= may be what you have.

I know a company that paid a considerable amount of money to am outside team to get the current release of GCC running on their no longer supported version of RHEL.


I am intrigued as to why the compiler feels it is allowed to simplify a function returning nothing as a no-op. After all this function allocates memory and stores things in there. This is a side effect that the optimization suppresses.


The C attitude is simple. If your code invokes undefined behavior, the compiler is allowed to do whatever it wants. Not returning a value from a function that is not void is definitely a footgun.

The C language itself does not prevent you from invoking these footguns. Why, I don't know for sure; I wasn't around for the genesis of C. I would guess that if C were designed today, most of the undefined behavior that can be detected statically would just be errors.

Of course, compilers are allowed to optimize assuming your code does not utilize undefined behavior. This is reasonable in my opinion. If the compiler were not allowed to optimize in ways that broke undefined behavior, it would be nearly impossible to write meaningful compiler optimizations. Most compiler optimizations change the semantics of the execution even though they keep the semantics of the program itself identical according to the rules of the C language. Where would you draw the line? If I monkey patch a function that my compiler decided to inline, would I have reason to complain?

In the face of C seeing your completely unused side-effects, it knows there is no possible way your program could ever actually touch those side-effects in the bounds of defined behavior. Therefore, it is free to throw them away. This may seem unreasonable, but in my opinion it's nearly the only reasonable choice.

To me the real solution is something like -Wall -Werror. For most statically detectable invocations of undefined behavior, this will save you, throwing an error in this case because "not all codepaths return a value." Then, you can sleep easy without sacrificing decent dead-code elimination.


That actually made sense.

I just wanted to point out that for me, C is more for embedded code, where modifying random parts of memory is a common task. So saying that a function that filled a pointer, used it to reset or set some parts of memory can be removed just because it does not have a return seems a bit extreme, but I agree that it is allowed.

I often see people pushing for -Wall and -Werror. In most professional projects I have been in, though, we have to use black boxes (or, currently, non-standard compilers) that prevent correcting many warnings. I wish we had a finer granularity there but well...


The compiler assumes that you have given it a valid C program, and optimizes accordingly.

A valid C program is not allowed to fall off the end of a non-void function without returning a value if the return value is used. This means that if the optimizer sees a function which will always do this, it "knows" that you never call it (as if you did, your program would be invalid). Since the function is never called, there's no point in emitting any code for the body of the function. It still needs to emit a small stub, as you could legally take the address of it.

As it so happens your program does call this function. This makes your program not a valid C program, and so the standard says nothing about how it should behave.


C is a language where you're expected to know what you're doing. While it wasn't necessarily right in this case, it's assumed that the generated assembly should match the intent of what you gave it.


The allocation’s address is only ever stored into a local variable. The compiler can therefore prove it’s not used elsewhere (short of e.g. scanning the whole heap for it)


>And this is also yet another demonstration, how C/C++ places return value into EAX/RAX register.

No, it's a compiler implementation detail, not a C/++ specification.


It's specified by the relevant ABI standard, FWIW.


'If you don't eat yer meat, you can't have any pudding.' No one mentions the change in attitude towards programming. If it wasn't correct YOU were not good enough.


It was always that way. The only difference is before the machines were so primitive that it wasn't an explicit decision on the designers part.


Being the designer I often wonder if that was my real fascination.


This gives me nightmares. I could spend weeks debugging this and not think of that.


You could spend two seconds turning on -Wall instead ;-)


What does clang do?


clang "<source>:19:1: warning: control reaches end of non-void function [-Wreturn-type]"

Given that no return is undefined behavior (6.6.3 c++ standard) this probably should just be treated as an error. (it would be nice if a manifested return was value/default init but it is not)

The reason why these are not errors in the compiler is for backwards compatibility.


Since the article is talking about C/C++

In C reaching the end of a non-void function is not undefined behaviour though. It's equivalent to ending with a return; However attempting to use the return value of that function is undefined behaviour. It'd be fine to have that warning be an error when compiling for C++, but not for C.

C89 3.6.6.4 (http://port70.net/~nsz/c/c89/c89-draft.html#3.6.6.4): "If a return statement without an expression is executed, and the value of the function call is used by the caller, the behavior is undefined. Reaching the } that terminates a function is equivalent to executing a return statement without an expression."


I’m sure that rule comes from the time when “void” didn’t exist, so functions were defined to return int, but nobody ever bothered to actually return anything.


It's definitely there to retain some backwards compatibility with existing K&R C programs. Though it does still somewhat exist in the C11 standards.

From 6.9.1 - "If the } that terminates a function is reached, and the value of the function call is used by the caller, the behavior is undefined"

Though 6.8.6.4 also says "A return statement without an expression shall only appear in a function whose return type is void"

So it seems like a non-void function hitting the end of the block without a return statement is allowed (provided the value isn't used). But having a "return;" in that function would not be in C11.


Classic example is printf. It returns an error, but nobody checks it.




Consider applying for YC's Spring batch! Applications are open till Feb 11.

Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: