dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
631 stars 180 forks source link

flatcc cli compiles with clang but not with gcc on Linux #231

Closed MartinKlang closed 2 years ago

MartinKlang commented 2 years ago

Current master branch doesn't build with gcc without -Wno-sign-conversion:

$ CC=gcc cmake ..
$ make
...
[ 16%] Building C object src/compiler/CMakeFiles/flatcc.dir/semantics.c.o
/home/mars/devel/evp-device-agent/evp_agent/sdkenc/flatcc/src/compiler/semantics.c: In function ‘analyze_struct’:
/home/mars/devel/evp-device-agent/evp_agent/sdkenc/flatcc/src/compiler/semantics.c:620:25: error: unsigned conversion from ‘int’ to ‘uint16_t’ {aka ‘short unsigned int’} changes the value of ‘-2’ [-Werror=sign-conversion]
  620 |     ct->symbol.flags &= ~fb_circular_open;
      |                         ^
cc1: all warnings being treated as errors
...

It does build with clang:

$ CC=clang cmake ..
$ make
...
[ 16%] Building C object src/compiler/CMakeFiles/flatcc.dir/semantics.c.o
[ 17%] Building C object src/compiler/CMakeFiles/flatcc.dir/coerce.c.o
[ 18%] Building C object src/compiler/CMakeFiles/flatcc.dir/flatcc.c.o
...
[100%] Built target bfbs2json

I'm on Ubuntu 22.04, gcc 11.2.0, clang 14.0.0, cmake 3.22.1, GNU Make 4.3

mikkelfj commented 2 years ago

Normally I would silence gcc warnings, but this error / warning is sort of correct since you can have a large flag that appears negative.

There are many places where flags are defined as integer enums that ought to have been unsigned so I'm not sure if it makes sense to change them all.

Just searching for fb_circular_open in repo also indicates that |= and & operators are being used on this flag but I don't know if you get errors here.

In the simplest case this can be fixed be casting just the problematic ~ operator.

How does it look on your side if you fix that line? And, are there more errors or warnings if you run scripts/test.sh and inspect output?

mikkelfj commented 2 years ago

Also, even if it is unsigned, it would still be a truncation to uint16_t on most platforms, so a cast is probably needed regardless.

MartinKlang commented 2 years ago

It's just that one line, if I cast to uint16_t then it builds correctly.

MartinKlang commented 2 years ago

With the cast, the tests pass

CXX=g++ CC=gcc scripts/test.sh
...
-- The C compiler identification is GNU 11.2.0
-- The CXX compiler identification is GNU 11.2.0
...
100% tests passed, 0 tests failed out of 22

Total Test time (real) =   0.46 sec
TEST PASSED
MartinKlang commented 2 years ago
diff --git a/src/compiler/semantics.c b/src/compiler/semantics.c
index 96d4b94..09b98d3 100644
--- a/src/compiler/semantics.c
+++ b/src/compiler/semantics.c
@@ -617,7 +617,7 @@ static int analyze_struct(fb_parser_t *P, fb_compound_type_t *ct)
     }

     ct->symbol.flags |= fb_circular_closed;
-    ct->symbol.flags &= ~fb_circular_open;
+    ct->symbol.flags &= (uint16_t)~fb_circular_open;
     ct->order = P->schema.ordered_structs;
     P->schema.ordered_structs = ct;
     return ret;
mikkelfj commented 2 years ago

Please test master, just to be sure, applied as suggested.

MartinKlang commented 2 years ago

yep that works, ty!

mikkelfj commented 2 years ago

Thanks