-
Notifications
You must be signed in to change notification settings - Fork 2k
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
native: fix C++ for FreeBSD #14631
native: fix C++ for FreeBSD #14631
Conversation
Looks good to me. There is no Maybe someone else can test this? On the other hand, Feel free to squash. |
Squashed |
Maybe @fjmolinas can also have a look regarding the saneness of build system integration. |
7cc0ac0
to
5befda3
Compare
At least I thought I did Oo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the use of DISABLE_LIBSTDCPP
, it replicates the same as FEATURES_USED
. If its broken then the feature should not be provided IMO, since the original PR introducing it was by @maribu, did you have a use case for this?
Anyway my comments are all about removing that variable.
ifeq (FreeBSD,$(OS)) | ||
DISABLE_LIBSTDCPP ?= 1 | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# libstdc++ on FreeBSD is broken (does not work with -m32)
ifneq (FreeBSD amd64, $(OS) $(OS_ARCH))
FEATURES_PROVIDED += libstdcpp
endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't make multiline comments but L8-12 would be removed.
ifeq (FreeBSD,$(OS)) | ||
ifeq ($(OS_ARCH),amd64) | ||
DISABLE_LIBSTDCPP ?= 1 | ||
endif | ||
ifeq ($(DISABLE_LIBSTDCPP),1) | ||
# static C++ constructors need guards for thread safe initialization | ||
ifneq (,$(filter cpp,$(FEATURES_USED))) | ||
USEMODULE += cxx_ctor_guards | ||
endif | ||
endif | ||
endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifeq (FreeBSD,$(OS)) | |
ifeq ($(OS_ARCH),amd64) | |
DISABLE_LIBSTDCPP ?= 1 | |
endif | |
ifeq ($(DISABLE_LIBSTDCPP),1) | |
# static C++ constructors need guards for thread safe initialization | |
ifneq (,$(filter cpp,$(FEATURES_USED))) | |
USEMODULE += cxx_ctor_guards | |
endif | |
endif | |
endif | |
ifeq (,$(filter libstdcpp,$(FEATURES_USED))) | |
# static C++ constructors need guards for thread safe initialization | |
ifneq (,$(filter cpp,$(FEATURES_USED))) | |
USEMODULE += cxx_ctor_guards | |
endif | |
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in all cases or only for FreeBSD? If its only for FreeBSD then the first ifdef should be kept as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only for FreeBSD 64-bit, yes. Otherwise this could be moved to a more global Makefile.dep
, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why so? Only arch using them so far is atmega? Why did you add it for FreeBSD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It provides the guards needed to call constructors during the initialization of statically allocated class instances. The C++ compiler will generate call to those utility functions. Usually those are implemented by the toolchain (e.g. as part of libstdc++). On FreeBSD the default libstdc++ is sadly not compatible with -m32
, so undefined references to those implementation will occur while linking. The module is a compatible drop-in replacement for platforms without (working) libstdc++, such as ATmega or FreeBSD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But true... with that knowledge having the condition (cpp && !libstdcpp)
in Makefile.dep
to include cxx_ctor_guards
makes little sense :-/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I keep it here. If I remove the dependency on DISABLE_LIBSTDCPP
I make dependent on @fjmolinas answer to #14631 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw: We might want to drop
cpu/esp32/cxx
in favor of the platform independentcxa_ctor_guards
.
Indeed, it's just a leftover required by former ESP32 toolchain versions that used POSIX threads. Although cxa_ctor_guards
were built-in, they had to be replaced since they used the pthread_once
function for singleton objects initialization where the parameter once
was of incompatible type with that provided by RIOT's pthread
module.
We are now using a self-compiled ESP32 toolchain version that no longer uses POSIX threads for thread-safety. That is, the toolchain does no longer guarantee thread-safety, as is the case with other platforms.
I think the ESP32 implementation is not correct. According to the documentation it is not thread safe, but thread safety was the very point those guards are there.
Aborting if a thread tries to access the variable during its initialization by another thread was the only way to get it to work at that time 😟
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cxx_ctor_guards
just use an rmutex for locking. That way the same thread is able to call the ctor of classes that call the ctor of other classes within their ctor and is thread safe. If those guards are indeed still needed, the module cxx_ctor_guards
should be a drop in replacement for every platform. (It only uses a single byte of whatever is passed as __guard
, so no matter how small this type is defined on a specific platform, it will still work.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
cxx_ctor_guards
just use an rmutex for locking.
Ah, there is a new module cxx_ctor_guards
. Sorry, I didn't notice that because I've been pretty busy the last few weeks rebasing and providing the new GPIO API after a half year break.
I don't remember exactly, but using a mutex for the ESP32 cxx_ctor_guards
led to deadlocks. Anyway, we can drop it now. I will provide a PR.
BTW my change suggestions are untested :) |
I am not sure what you mean by that. From what I can tell, your suggestions don't allow for FreeBSD users to use |
Mhhhh I just noticed, while this compiles, many tests (not just C++ tests) are segfaulting now with the current version of this PR. I can't debug this however due to Lines 650 to 658 in 0e3aa2f
and
|
Just removing that makes |
:-/ |
With #14631 (comment) addressed at least the non-C++ tests work again :D |
As I said my changes suggestions where only about removing the |
Thanks for the explanation @fjmolinas. I might have found another solution I stumbled upon when researching #14631 (comment). I will try if that maybe solves the linking problems altogether. |
+1 |
Apparently this is how the vagrant box is already configured :-/. No idea how to select the right
but no idea how to convince the |
I think you can invoke E.g. on Alpine I can run
|
Maybe FreeBSD has something link binfmt on Linux, so that you can convince it to automagically run 32 bit apps correctly? |
Please be more specific on that. |
Sorry, it is I successfully used this concept to run ELF files compiler for other targets on my notebook using qemu. E.g. I can just run I'm confident that this approach - if supported by FreeBSD - would work to run 32 bit binary with the correctly |
I'm not really sure where I am supposed to put that :-/. |
Either my internet search skills are poor, or FreeBSD does not support binfmt_misc. Sorry |
I am actually also confused where to put that in Linux? Are they commands? Are those configurations? Where do those words you said go 😅 Maybe if I know what to do in Linux I can work from there in FreeBSD ;-). |
It is a configuration interface telling the kernel how to execute non-native binary executables. The details are described e.g. here. I'm not 100% sure, but it seems that FreeBSD doesn't have this feature. (Or my internet-search-fu is to bad to find it.) So the easiest might just be to adapt the |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
Cross-compiling C++ for 32-bit on a 64-bit FreeBSD is not as easy as installing the respective lib via package management as it is in most Linux distributions. This configures the build system to honor the `libstdcpp` feature for 64-bit FreeBSD systems.
76b9a8c
to
ad1348c
Compare
Rebased.
The problem is that the linking error occurs duirng
Given that I do not really know how proceed with this and also somewhat lost track, I'd say: let's close it for now. |
Contribution description
Cross-compiling C++ for 32-bit on a 64-bit FreeBSD is not as easy as installing the respective lib via package management as it is in most Linux distributions. This configures the build system to honor the
libstdcpp
feature for 64-bit FreeBSD systems.Testing procedure
Merge #14458, install vagrant on your system by following the README provided there and do
Without this PR, the test won't link, with it it will. You can also test other C++ tests. Most of them will skip compilation because the
libstdcpp
feature is required, but those who do not will successfully link.Compiling with
DISABLE_LIBSTDCPP=0
in the environment should still fail linking.Issues/PRs references
Found in #14503, requires #14458 for testing but not to be merged.