Page MenuHomeFreeBSD

Fix gcc build for superio(4)
ClosedPublic

Authored by lwhsu on Jul 8 2019, 1:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 7:14 AM
Unknown Object (File)
Sat, Nov 30, 11:53 PM
Unknown Object (File)
Tue, Nov 26, 12:36 PM
Unknown Object (File)
Tue, Nov 26, 10:59 AM
Unknown Object (File)
Nov 19 2024, 1:27 PM
Unknown Object (File)
Nov 19 2024, 1:25 AM
Unknown Object (File)
Nov 11 2024, 6:07 PM
Unknown Object (File)
Sep 28 2024, 11:07 AM
Subscribers

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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25246
Build 23915: arc lint + arc unit

Event Timeline

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

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

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?

sys/dev/superio/superio.c
408

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

418

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.

sys/dev/superio/superio.c
418

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.

sys/dev/superio/superio.c
418

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"?

sys/dev/superio/superio.c
418

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.

sys/dev/superio/superio.c
418

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
%
sys/dev/superio/superio.c
418

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.

  • Change string mapping of SUPERIO_DEV_NONE (to distinguish from SUPERIO_DEV_MAX)
  • Drop unnecessary comment
lwhsu marked 2 inline comments as done.
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.