dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
651 stars 189 forks source link

Build warning with embedded toolchain (Espressif esp-idf) #77

Closed paulreimer closed 6 years ago

paulreimer commented 6 years ago
In file included from ./lib/flatcc/include/flatcc/flatcc_flatbuffers.h:24:0,
                 from ./lib/flatcc/include/flatcc/flatcc_builder.h:68,
                 from ./src/json_to_flatbuffers_converter.h:20,
                 from ../main/rxcpp_test_task.cpp:21:
./lib/flatcc/include/flatcc/portable/pstdalign.h:36:0: warning: "_Alignas" redefined
 #define _Alignas(t) __attribute__((__aligned__(t)))

Similar with _Alignof.

In my file which includes flatcc headers directly (for generic JSON parsing), I just include these files without any other flags/guards:

#include "flatcc/flatcc_builder.h"
#include "flatcc/flatcc_json_parser.h"
#include "flatcc/flatcc_verifier.h"
mikkelfj commented 6 years ago

As I recall from other issue https://github.com/dvidelabs/flatcc/pull/73#issuecomment-357337934 this is specific to C++14 because the stdalign is no longer a macro.

Could you please help figure out why the proposed change to the pstdalign.h did not work for you?

https://github.com/dvidelabs/flatcc/tree/alignas_cplusplus https://github.com/dvidelabs/flatcc/commit/1f1661c0106e4f1a70e7b3053e71b3616f9df5b4

paulreimer commented 6 years ago

Ah right, sorry. I had tried that branch during the previous C++ work, but it didn't work at the time. ~Now that branch, updated with the latest master merged in, works fine for me!~

paulreimer commented 6 years ago

Haha wait a sec. That branch does remove the error for my C++ files which include the flatcc files.

~BUT it introduces~ However, this is still a warning now when compiling the flatcc C-files in src/runtime/.

In file included from ./lib/flatcc/include/flatcc/flatcc_flatbuffers.h:24:0,
                 from ./lib/flatcc/include/flatcc/flatcc_builder.h:68,
                 from ./lib/flatcc/src/runtime/builder.c:21:
./lib/flatcc/include/flatcc/portable/pstdalign.h:48:0: warning: "_Alignas" redefined
 #define _Alignas(t) __attribute__((__aligned__(t)))
 ^
In file included from /esp-idf/components/newlib/include/stdlib.h:19:0,
                 from ./lib/flatcc/src/runtime/builder.c:16:
/esp-idf/components/newlib/include/sys/cdefs.h:281:0: note: this is the location of the previous definition
 #define _Alignas(x)  __aligned(x)
 ^
In file included from ./lib/flatcc/include/flatcc/flatcc_flatbuffers.h:24:0,
                 from ./lib/flatcc/include/flatcc/flatcc_builder.h:68,
                 from ./lib/flatcc/src/runtime/builder.c:21:
./lib/flatcc/include/flatcc/portable/pstdalign.h:49:0: warning: "_Alignof" redefined
 #define _Alignof(t) __alignof__(t)
 ^
In file included from /esp-idf/components/newlib/include/stdlib.h:19:0,
                 from ./lib/flatcc/src/runtime/builder.c:16:
/esp-idf/components/newlib/include/sys/cdefs.h:288:0: note: this is the location of the previous definition
 #define _Alignof(x)  __alignof(x)
 ^
CC build/request_manager/lib/flatcc/src/runtime/json_parser.o
In file included from ./lib/flatcc/include/flatcc/flatcc_flatbuffers.h:24:0,
                 from ./lib/flatcc/include/flatcc/flatcc_builder.h:68,
                 from ./lib/flatcc/include/flatcc/flatcc_json_parser.h:20,
                 from ./lib/flatcc/src/runtime/json_parser.c:2:
./lib/flatcc/include/flatcc/portable/pstdalign.h:48:0: warning: "_Alignas" redefined
 #define _Alignas(t) __attribute__((__aligned__(t)))
paulreimer commented 6 years ago

And the file at which has the other definition: https://github.com/espressif/esp-idf/blob/master/components/newlib/include/sys/cdefs.h#L273-L289

mikkelfj commented 6 years ago

OK, my gut feeling as that it is an issue with cdefs because it doesn't check if __alignas_is_defined is defined. It just assumes that it isn't defined if a given compiler version is not present. But that is from a 5 sec inspection. If true, it is pretty hard to work around such libraries.

paulreimer commented 6 years ago

So just to confirm, but you suspect this to be an issue with that file in the esp-idf repo, then? They are open to PRs, I can at least report it. Just adding a simple guard for that __alignas_is_defined, then?

mikkelfj commented 6 years ago

I don't think this is something flatcc/portable should try to support or work around. Instead you can define __alignas_is_defined=1 in the build configuration of your project and/or convince cdefs upstream to do it.

mikkelfj commented 6 years ago

But, I can include the changes in the branched linked above, now that they are confirmed to be effective.

mikkelfj commented 6 years ago

Just adding a simple guard for that __alignas_is_defined, then?

They should add a guard and define it if the guard fails. That would make it interoperate.

mikkelfj commented 6 years ago

The C++14 fix has been added to master branch.

mikkelfj commented 6 years ago

After further inspection I'm not sure it is sufficient to guard and define __alignas_is_defined=1 in cdefs.h by itself because alignas and alignof is not defined. These macros should also be defined.

paulreimer commented 6 years ago

Well, I ran it up the flagpole over at esp-idf and one of the maintainers thinks it should be a flatcc responsibility to handle this newlib behaviour (the C-library used here instead of glibc). He also gave a suggestion: https://github.com/espressif/esp-idf/issues/1585#issuecomment-363619761

mikkelfj commented 6 years ago

@paulreimer Could you try to include before including flatcc headers in your project without manually defining __alignas_is_defined, and report the result here?

mikkelfj commented 6 years ago

@paulreimer Could you also try this branch and see if it works unmodified, and if not, if it works with HAVE_STDALIGN_H defined. https://github.com/dvidelabs/flatcc/tree/issue-77

paulreimer commented 6 years ago

@mikkelfj I can confirm that branch works without warnings (C files in src/runtime/ and my own C++ files), and without needing any modifications or flags/overrides! (So, I guess flatcc supports C++ w/newlib now!)

mikkelfj commented 6 years ago

Thanks for the confirmation, merged into master.