Page MenuHomeFreeBSD

Fix gcc build for superio(4)
ClosedPublic

Authored by lwhsu on Jul 8 2019, 1:16 PM.

Details

Summary

On https://ci.freebsd.org/job/FreeBSD-head-amd64-gcc/10595/console :

--- all_subdir_superio ---
/workspace/src/sys/modules/superio/../../dev/superio/superio.c: In function 'devtype_to_str':
/workspace/src/sys/modules/superio/../../dev/superio/superio.c:418:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^
Test Plan

make -DNO_CLEAN CROSS_TOOLCHAIN=amd64-gcc buildworld buildkernel

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lwhsu created this revision.Jul 8 2019, 1:16 PM
avg added a comment.Jul 8 2019, 1:22 PM

I did not add the default case, because I wanted a compiler to let me know when I forget to add a new type to that switch.
It sucks that the gcc does not realize that the code after the switch is not reachable.
But I guess that we have to please it.

sys/dev/superio/superio.c
416 ↗(On Diff #59534)

How about adding a new return statement after the switch instead of 'default label?

lwhsu added a comment.Jul 8 2019, 3:12 PM

Agreed, I also think this may be a problem of the old GCC, haven't tried with newer one (which should have been using) yet.

I'll use a new return statement and let's also add a comment that this is only for the old GCC. How do you think?

avg added a comment.Jul 8 2019, 3:17 PM

I agree.
Thank you!

lwhsu updated this revision to Diff 59536.Jul 8 2019, 3:25 PM

Update per @avg's suggestion.

imp added inline comments.Jul 8 2019, 5:06 PM
sys/dev/superio/superio.c
408 ↗(On Diff #59536)

I'd return "none" or "none-specified" here.

418 ↗(On Diff #59536)

I'd be tempted to just lose this comment. It adds no value and is wrong.
there's no such thing as an enum type that can have only certain values in C, so the compiler isn't wrong for complaining.
there's no way for the compiler to know this will never be called with an invalid value.

jhb added inline comments.Jul 8 2019, 5:26 PM
sys/dev/superio/superio.c
418 ↗(On Diff #59536)

Modern compilers haven't agreed with you in the last 5 years or so. They definitely assume that if you have 'enum foo' you can only ever assign values defined by that type to the enum and not other values. You have to add explicit (int) casts if you need to treat it as a raw int now. So modern compilers will assume that if you've handled all the defined enum values, there won't be a default case. GCC 4.2 is the only compiler capable of compiling FreeBSD that will trip over this.

lwhsu added inline comments.Jul 8 2019, 5:57 PM
sys/dev/superio/superio.c
418 ↗(On Diff #59536)

Well, but this build is using CROSS_TOOLCHAIN=amd64-gcc, which uses amd64-gcc-6.4.0_5 at this point.

Adding a return statement seems trivial to solve the compilation issue here, but is there any way works for @avg 's requirement: "let compiler to report when forgetting to add a new type to that switch"?

jhb added inline comments.Jul 8 2019, 6:00 PM
sys/dev/superio/superio.c
418 ↗(On Diff #59536)

Oof, I had read the comment as only being 4.2. I would also remove the comment if GCC 6 still can't cope. I don't know of a good way to achieve @avg's requirement. We could maybe put the return under #ifndef clang but that's kind of ugly.

imp added inline comments.Jul 8 2019, 6:03 PM
sys/dev/superio/superio.c
418 ↗(On Diff #59536)

Not sure that you are correct John:

% cat foo.c
enum foo { a = 1, b = 2, c = 4 };
enum foo x;

void bar(void)
{
	x = a | c;
}
% clang -Wall foo.c -c
% clang -v
clang -v
FreeBSD clang version 8.0.0 (tags/RELEASE_800/final 356365) (based on LLVM 8.0.0)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin
%
jhb added inline comments.Jul 8 2019, 6:08 PM
sys/dev/superio/superio.c
418 ↗(On Diff #59536)

Actually, no need for the #ifndef, I forgot the return wasn't in a default: so it should keep the warning @avg cares about. I would just drop the comment though since it isn't just GCC 4.2.

lwhsu updated this revision to Diff 59546.Jul 8 2019, 7:40 PM
  • Change string mapping of SUPERIO_DEV_NONE (to distinguish from SUPERIO_DEV_MAX)
  • Drop unnecessary comment
lwhsu added a reviewer: imp.Jul 8 2019, 7:40 PM
lwhsu marked 2 inline comments as done.
imp accepted this revision.Jul 8 2019, 7:46 PM

thanks!

This revision is now accepted and ready to land.Jul 8 2019, 7:46 PM
This revision was automatically updated to reflect the committed changes.