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

_Float16 undeclared when compiling on aarch64 #178

Closed
Scrumplex opened this issue Nov 15, 2022 · 12 comments
Closed

_Float16 undeclared when compiling on aarch64 #178

Scrumplex opened this issue Nov 15, 2022 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@Scrumplex
Copy link
Contributor

Scrumplex commented Nov 15, 2022

Environment

toml++ version and/or commit hash: cc741c9

Compiler: GNU 12.1.0

C++ standard mode: 17

Target arch: aarch64 (arm64)

Library configuration overrides: None

Relevant compilation flags: None?

Describe the bug

Building toml++ on aarch64 results in the following error:

In file included from /run/build/prismlauncher/libraries/tomlplusplus/include/toml++/toml.h:13,
                 from /run/build/prismlauncher/launcher/modplatform/packwiz/Packwiz.cpp:31:
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:630:29: error: ‘_Float16’ was not declared in this scope; did you mean ‘_Float64’?
  630 |         struct float_traits<TOML_FLOAT16> : float_traits_base<TOML_FLOAT16, __FLT16_MANT_DIG__, __FLT16_DIG__>
      |                             ^~~~~~~~~~~~
In file included from /run/build/prismlauncher/libraries/tomlplusplus/include/toml++/toml.h:39:
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:630:41: error: template argument 1 is invalid
  630 |         struct float_traits<TOML_FLOAT16> : float_traits_base<TOML_FLOAT16, __FLT16_MANT_DIG__, __FLT16_DIG__>
      |                                         ^
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:630:63: error: ‘_Float16’ was not declared in this scope; did you mean ‘_Float64’?
  630 |         struct float_traits<TOML_FLOAT16> : float_traits_base<TOML_FLOAT16, __FLT16_MANT_DIG__, __FLT16_DIG__>
      |                                                               ^~~~~~~~~~~~
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:630:110: error: template argument 1 is invalid
  630 |         struct float_traits<TOML_FLOAT16> : float_traits_base<TOML_FLOAT16, __FLT16_MANT_DIG__, __FLT16_DIG__>
      |                                                                                                              ^
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:651:29: error: ‘_Float16’ was not declared in this scope; did you mean ‘_Float64’?
  651 |         struct value_traits<TOML_FLOAT16> : float_traits<TOML_FLOAT16>
      |                             ^~~~~~~~~~~~
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:651:41: error: template argument 1 is invalid
  651 |         struct value_traits<TOML_FLOAT16> : float_traits<TOML_FLOAT16>
      |                                         ^
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:651:58: error: ‘_Float16’ was not declared in this scope; did you mean ‘_Float64’?
  651 |         struct value_traits<TOML_FLOAT16> : float_traits<TOML_FLOAT16>
      |                                                          ^~~~~~~~~~~~
/run/build/prismlauncher/libraries/tomlplusplus/include/toml++/impl/forward_declarations.h:651:70: error: template argument 1 is invalid
  651 |         struct value_traits<TOML_FLOAT16> : float_traits<TOML_FLOAT16>
      |             

Note that this issue does not occur on x86_64 with the same toolchain.

Steps to reproduce (or a small repro code sample)

Best I can offer is the Flatpak builder here: flathub/org.prismlauncher.PrismLauncher#6

Additional information

Full build log: https://buildbot.flathub.org/#/builders/42/builds/565/steps/8/logs/stdio

@Scrumplex Scrumplex added the bug Something isn't working label Nov 15, 2022
@Scrumplex
Copy link
Contributor Author

I have downgraded back to 3.2.0 for now

@Scrumplex
Copy link
Contributor Author

I am assuming that c8780a5 introduced this, but I don't have an aarch64 machine to test on

@marzer
Copy link
Owner

marzer commented Nov 15, 2022

That commit was an effort to fix fp16-related issues, though I guess I made it worse for some targets. I recently ran into similar issues in another project and have been meaning to backport my findings to toml++. Should be relatively easy to sort out. (I'll add an escape hatch so you can disable all that stuff anyway.)

@eddelbuettel
Copy link

Adding to this close / stale issue as we see this with the 3.2.0 code:

Should we just try to build of the master branch? Or are you planning a release soon?

@marzer
Copy link
Owner

marzer commented Jan 24, 2023

Should we just try to build of the master branch? Or are you planning a release soon?

It will be safe to use the current master, but I will be doing a release in the next week or so if you'd prefer to wait - there's a few other small issues on the backlog that I would like to include.

One of them, #187, is a TOML-language spec compliance issue. Admittedly a pretty weird edge-case, but I'd like to solve it all the same.

@eddelbuettel
Copy link

Hm. Apparently I already took you master branch code when I bootstrapped my repo of rcpptoml around it. The only difference is in preprocessor.h (my copy so from Jan 1):

edd@rob:~/git/tomlplusplus(master)$ git pull --all
Already up to date.
edd@rob:~/git/tomlplusplus(master)$ rsync -ncav include/toml++/ ../rcpptomlplusplus/inst/include/toml++/
sending incremental file list
impl/
impl/preprocessor.h

sent 2,091 bytes  received 19 bytes  4,220.00 bytes/sec
total size is 636,899  speedup is 301.85 (DRY RUN)
edd@rob:~/git/tomlplusplus(master)$ 

Will give that a shot.

@marzer
Copy link
Owner

marzer commented Jan 24, 2023

Yup, I just removed my attempts at trying to automatically detect _Float16 support - now it is entirely opt-in via TOML_ENABLE_FLOAT16. Despite my best intentions, compiler bugs kept killing me here.

@eddelbuettel
Copy link

eddelbuettel commented Jan 24, 2023

Yeah. Saw that. I am not quite sure how to handle that, probably need a preprocessor. We can upload to an arm test builder at the R Project, and I just did that (and rushed at first, fooled myself locally) and it passes. But so does the prior version on that machine as that build is 64 bit.

Should I bail and enable TOML_ENABLE_FLOAT16 "everywhere" just in case?

@marzer
Copy link
Owner

marzer commented Jan 24, 2023

Hmmn? I'm confused. If you enable it everywhere you'll get the same compiler issues you got in 3.2.0.

The thing I removed was the various preprocessor #ifdefs that tried to detect _Float16 support based on the compiler and arch; now it's entirely opt-in such that if you want toml++ to be aware of _Float16, you need to explicitly set TOML_ENABLE_FLOAT16=1, but doing so on a compiler/arch that does not support it will cause issues.

The only thing it impacts is the ability to directly get/set values as _Float16, e.g.:

std::optional<_Float16> f16_val = some_node.value<_Float16>();

Which is a pretty niche thing, and unlikely to be useful for you given you're wrapping the library anyways. My recommendation would be to explicitly set TOML_ENABLE_FLOAT16=0.

@eddelbuettel
Copy link

I am confused to. I build RcppTOML in early Jan off your repo then, I should have most fixes but still have the now-removed heuristic of yours to see when to enable float16 support. That is what I shipped to CRAN, it clearly fails on two platforms: https://cloud.r-project.org/web/checks/check_results_RcppTOML.html My fellow Debianer looking after my (usptream) package also has woes.

I then confused myself once more by using the machine accessible at https://mac.r-project.org/macbuilder/submit.html but the CRAN arm64 aka aarch64 build already passed, it was the plain old macOS that failed per the table in
https://cloud.r-project.org/web/checks/check_results_RcppTOML.html
with log at
https://www.r-project.org/nosvn/R.check/r-release-macos-x86_64/RcppTOML-00install.html
and that looks like a ... compiler issue / the choice of
clang++ -mmacosx-version-min=10.13 -std=gnu++17
creating woes. So different issue.

Back to float16. So when/where should I enable? Only on i386? Why does arm64 fail at Debian in
https://buildd.debian.org/status/fetch.php?pkg=r-cran-rcpptoml&arch=arm64&ver=0.2.0%2Bdfsg-1&stamp=1674418861&raw=0 (and scroll to bottom) when it is 64 bit?

@eddelbuettel
Copy link

My recommendation would be to explicitly set TOML_ENABLE_FLOAT16=0.

I am extra dense today. Thank you.

I thought our woes were from wanting to enable rather than explicitly disable where it may fail.

@eddelbuettel
Copy link

Small wins. The 'win-builder' R Project machine, in its 'support previous release of R' setting where it still builds with i386 and x86_64 flavours (current release dropped i386) fared well once I added the define. See https://win-builder.r-project.org/fEJ9pDGItp63/00install.out and copying 'for keeps' as that page will get purhed in a few days:

* installing *source* package 'RcppTOML' ...
** using staged installation
** libs

*** arch - i386
d:/Compiler/rtools40/mingw32/bin/g++  -std=gnu++17 -I"D:/RCompile/recent/R-4.1.3/include" -DNDEBUG -I../inst/include -DTOML_ENABLE_FLOAT16=0 -I'd:/RCompile/CRANpkg/lib/4.1/Rcpp/include'   -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign  -c RcppExports.cpp -o RcppExports.o
d:/Compiler/rtools40/mingw32/bin/g++  -std=gnu++17 -I"D:/RCompile/recent/R-4.1.3/include" -DNDEBUG -I../inst/include -DTOML_ENABLE_FLOAT16=0 -I'd:/RCompile/CRANpkg/lib/4.1/Rcpp/include'   -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign  -c parse.cpp -o parse.o
d:/Compiler/rtools40/mingw32/bin/g++ -shared -s -static-libgcc -o RcppTOML.dll tmp.def RcppExports.o parse.o -Ld:/Compiler/gcc-4.9.3/local330/lib/i386 -Ld:/Compiler/gcc-4.9.3/local330/lib -LD:/RCompile/recent/R-4.1.3/bin/i386 -lR
installing to d:/RCompile/CRANguest/R-oldrelease/lib/00LOCK-RcppTOML/00new/RcppTOML/libs/i386

*** arch - x64
d:/Compiler/rtools40/mingw64/bin/g++  -std=gnu++17 -I"D:/RCompile/recent/R-4.1.3/include" -DNDEBUG -I../inst/include -DTOML_ENABLE_FLOAT16=0 -I'd:/RCompile/CRANpkg/lib/4.1/Rcpp/include'   -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign  -c RcppExports.cpp -o RcppExports.o
d:/Compiler/rtools40/mingw64/bin/g++  -std=gnu++17 -I"D:/RCompile/recent/R-4.1.3/include" -DNDEBUG -I../inst/include -DTOML_ENABLE_FLOAT16=0 -I'd:/RCompile/CRANpkg/lib/4.1/Rcpp/include'   -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign  -c parse.cpp -o parse.o
d:/Compiler/rtools40/mingw64/bin/g++ -shared -s -static-libgcc -o RcppTOML.dll tmp.def RcppExports.o parse.o -Ld:/Compiler/gcc-4.9.3/local330/lib/x64 -Ld:/Compiler/gcc-4.9.3/local330/lib -LD:/RCompile/recent/R-4.1.3/bin/x64 -lR
installing to d:/RCompile/CRANguest/R-oldrelease/lib/00LOCK-RcppTOML/00new/RcppTOML/libs/x64
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
*** arch - i386
*** arch - x64
** testing if installed package can be loaded from final location
*** arch - i386
*** arch - x64
** testing if installed package keeps a record of temporary installation path
* MD5 sums
packaged installation of 'RcppTOML' as RcppTOML_0.2.0.1.zip
* DONE (RcppTOML)

That is with the updated preprocessor.h and with -DTOML_ENABLE_FLOAT16=0 (as shown).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants