Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

baremetal: amalgamated header missing #include <stdarg.h> #193

Open
danngreen opened this issue Jan 19, 2022 · 3 comments
Open

baremetal: amalgamated header missing #include <stdarg.h> #193

danngreen opened this issue Jan 19, 2022 · 3 comments

Comments

@danngreen
Copy link

The output of the amalgamate.py script produces a file which contains all instances of #include <stdarg.h> commented out, except for the first one. This would be fine except that the first instance of #include <stdarg.h> is inside a #ifdef C4CORE_SINGLE_HDR_DEFINE_NOW block, but the second #include <stdarg.h> (which is imported from src/c4/yml/parse.hpp) is not within a #ifdef C4CORE_SINGLE_HDR_DEFINE_NOW or #ifdef RYML_SINGLE_HDR_DEFINE_NOW block.

Therefore, when including the amalgamated output into a file without a preceding #define RYML_SINGLE_HDR_DEFINE_NOW, the va_list symbol is used without being declared, and produces an error (on at least some compilers). This is of course regardless of whether or not another compilation unit included the amalgamated output with RYML_SINGLE_HDR_DEFINE_NOW defined, since it's a compilation error, not a linker error.

On the arm-none-eabi-gcc toolchain, I get this error, but compiling with clang++ 13 seems to work. I don't know the reason for the difference, but I'm guessing it's why this hasn't been detected until now.

Here's an easy way to replicate: (this is assuming the gcc-arm embedded toolchain 10.3 is installed and on $PATH):

git clone --recursive https://github.com/biojppm/rapidyaml
python3 rapidyaml/tools/amalgamate.py > ryml_all.hpp
echo "#include \"ryml_all.hpp\"" > test.cpp
arm-none-eabi-g++ -std=c++20 -c test.cpp -o test.o

Produces this error:

In file included from test.cpp:1:
ryml_all.hpp:20716:59: error: 'va_list' has not been declared
20716 |     int  _fmt_msg(char *buf, int buflen, const char *msg, va_list args) const;
      |                                                           ^~~~~~~

Including <stdarg.h> before ryml_all.hpp works:

cat <<EOF > test2.cpp
#include <stdarg.h>
#include "ryml_all.hpp"
EOF
arm-none-eabi-g++ -std=c++20 -c test2.cpp -o test2.o

I would assume all standard library headers will have include guards, so I'm guessing the reason to ignore duplicate includes is to speed up compile time when using the amalgamated header? I can see it being tricky to get the python script to detect which #includes are within #ifdef blocks, and comment out accordingly, so I don't have a proposal for an easy, general solution. It's not a difficult work-around to include <stdarg.h> manually. But I did want to bring it up since it might help someone else also using this project in the single-header method with the gcc arm compiler (since it is, IMO an excellent tool for using yaml on embedded systems).

Thanks for you hard work on this project.

@biojppm
Copy link
Owner

biojppm commented Jan 19, 2022

Thanks for the detailed write-up. Indeed the amalgamation tool is removing repeated includes to keep compilation times reduced. And indeed the tool is dumb and cannot detect #ifdef context around those includes. I plan on improving the tool to handle this situation, but cannot commit to doing it right now. For the moment I will add a chunk with #include <stdarg.h> when amalgamating. That seems the best workaround for now.

@danngreen
Copy link
Author

Makes sense. Thanks again!

@biojppm biojppm changed the title Amalgamated header comments out a required #include baremetal: amalgamated header comments out a required #include Jan 21, 2022
@biojppm biojppm changed the title baremetal: amalgamated header comments out a required #include baremetal: amalgamated header missing #include <stdarg.h> Jan 21, 2022
@biojppm
Copy link
Owner

biojppm commented Jan 21, 2022

@danngreen The fixes were merged to master. I will keep this issue open (and biojppm/c4core#63 as well) until there is a QEMU in the CI in either repo proving that code does indeed work with arm-none-eabi. For now, in the absence of such proof, please let me know if you find any other problem or quirk. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants