search

sparcians / map

Modeling Architectural Platform
Apache License 2.0
156 stars 56 forks source link

cmake files linking to `-lpthread` but not passing `-pthread` option isn't correct for GCC. Need `find_module(Threads REQUIRED)` #485

Open timsnyder opened 5 months ago

timsnyder commented 5 months ago

https://github.com/sparcians/map/blob/ee08219c9f3bf395c648720e0fdada4eb4b0c06f/sparta/cmake/sparta-config.cmake#L84 (and the same thing in simdb-config.cmake)

Mean that whenever someone builds with GCC instead of clang, we're doing the wrong thing. I haven't seen problematic behavior yet, I'm just scrubbing the build scripts to resolve some issues in my downstream model.

The way pthread is being included in the build is maybe not 100% correct for GCC. We're passing -lpthread to linking but not passing the -pthread option to compile. This means that the _REENTRANT and _USE_REENTRANT macros are not defined and I read somewhere (that of course I'm having trouble finding again) that not having those macros defined at compile time means that appropriate locking isn't happening and I really have no clue why this isn't manifesting with ugly issues. Perhaps I'm not really pushing the concurrency features of Sparta hard? As I've mentioned elsewhere, I'm not using simdb yet.

https://stackoverflow.com/questions/23250863/difference-between-pthread-and-lpthread-while-compiling explains some of this stuff.

https://stackoverflow.com/questions/1620918/cmake-and-libpthread describes the "correct" way to configure cmake to use pthread so that the compile-time option is included as well as the link-time option and would react to different toolchains being used.

I'll try to push a PR for this but didn't want to hold off on filing a ticket capturing the issue.