cubing / twsearch

🔍 Twizzle Search — a program to find algs and scrambles for twisty puzzles
GNU General Public License v3.0
27 stars 9 forks source link

GCC build needs '__builtin_' prefix on ffsll() and strdup() #41

Closed DougCube closed 1 year ago

DougCube commented 1 year ago

When building with GCC (latest version), there were 11 instances in the codebase where I had to make this change to let it go thru, perhaps there's some way to adjust the code so this is not needed, like with some preprocessor macro detecting which compiler is being used.

Or that there is some option for GCC I'm not aware of and the makefile could be adjusted to pass it an option that would take care of this.  

diff --git a/src/cpp/ffi/ffi_api.cpp b/src/cpp/ffi/ffi_api.cpp
index 9e44f72..e6a388f 100644
--- a/src/cpp/ffi/ffi_api.cpp
+++ b/src/cpp/ffi/ffi_api.cpp
@@ -47,14 +47,14 @@ void ffi_api_set_arg(string s) {
   while (space < (int)s.size() && s[space] != ' ')
     space++;
   if (space >= (int)s.size()) {
-    argv[1] = strdup(s.c_str());
+    argv[1] = __builtin_strdup(s.c_str());
     argv[2] = 0;
     argc = 2;
   } else {
     scopy = s;
     scopy[space] = 0;
-    argv[1] = strdup(scopy.c_str());
-    argv[2] = strdup(scopy.c_str() + space + 1);
+    argv[1] = __builtin_strdup(scopy.c_str());
+    argv[2] = __builtin_strdup(scopy.c_str() + space + 1);
     argv[3] = 0;
     argc = 3;
   }
diff --git a/src/cpp/god.cpp b/src/cpp/god.cpp
index 85a60bb..4bcbf1c 100644
--- a/src/cpp/god.cpp
+++ b/src/cpp/god.cpp
@@ -46,7 +46,7 @@ void dotwobitgod(puzdef &pd) {
         checkv = (checkv & 0x5555555555555555LL) &
                  ((checkv >> 1) & 0x5555555555555555LL);
 #ifdef HAVE_FFSLL
-        for (int smi = ffsll(checkv); checkv; smi = ffsll(checkv)) {
+        for (int smi = __builtin_ffsll(checkv); checkv; smi = __builtin_ffsll(checkv)) {
 #else
         for (int smi = 1; checkv; smi++) {
           if (0 == ((checkv >> (smi - 1)) & 1))
@@ -78,7 +78,7 @@ void dotwobitgod(puzdef &pd) {
         checkv = (checkv & 0x5555555555555555LL) &
                  ((checkv >> 1) & 0x5555555555555555LL);
 #ifdef HAVE_FFSLL
-        for (int smi = ffsll(checkv); checkv; smi = ffsll(checkv)) {
+        for (int smi = __builtin_ffsll(checkv); checkv; smi = __builtin_ffsll(checkv)) {
 #else
         for (int smi = 1; checkv; smi++) {
           if (0 == ((checkv >> (smi - 1)) & 1))
diff --git a/src/cpp/puzdef.h b/src/cpp/puzdef.h
index a242dbf..94cd2ba 100644
--- a/src/cpp/puzdef.h
+++ b/src/cpp/puzdef.h
@@ -232,12 +232,12 @@ struct puzdef {
     for (int o = 1; o < setdefs[0].size; o++) {
       if ((r & (r - 1)) == 0)
         return r;
-      int t = ffsll(r) - 1;
+      int t = __builtin_ffsll(r) - 1;
       ull r2 = 1LL << t;
       int rv = rotinvmap[t].dat[bp[rotgroup[t].pos.dat[o]]];
       r &= ~(1LL << t);
       while (r) {
-        int m = ffsll(r) - 1;
+        int m = __builtin_ffsll(r) - 1;
         r &= ~(1LL << m);
         t = rotinvmap[m].dat[bp[rotgroup[m].pos.dat[o]]];
         if (t < rv) {
diff --git a/src/cpp/rotations.cpp b/src/cpp/rotations.cpp
index 77ca971..d57239c 100644
--- a/src/cpp/rotations.cpp
+++ b/src/cpp/rotations.cpp
@@ -295,11 +295,11 @@ int slowmodm2(const puzdef &pd, const setval p1, setval p2) {
   int cnt = 1;
   if (pd.rotgroup.size() <= 64) {
     ull lobits = pd.lowsymmbits(p1);
-    int g = ffsll(lobits) - 1;
+    int g = __builtin_ffsll(lobits) - 1;
     pd.mul3(pd.rotinvmap[g], p1, pd.rotgroup[g].pos, p2);
     lobits &= ~(1LL << g);
     while (lobits) {
-      g = ffsll(lobits) - 1;
+      g = __builtin_ffsll(lobits) - 1;
       lobits &= ~(1LL << g);
       int t = pd.mulcmp3(pd.rotinvmap[g], p1, pd.rotgroup[g].pos, p2);
       if (t <= 0) {
rokicki commented 1 year ago

What platform were you building on and what version of gcc?

On Wed, Nov 8, 2023 at 1:00 PM DougCube @.***> wrote:

When building with GCC (latest version), there were 11 instances in the codebase where I had to make this change to let it go thru, perhaps there's some way to adjust the code so this is not needed, like with some preprocessor macro detecting which compiler is being used.

Or that there is some option for GCC I'm not aware of and the makefile could be adjusted to pass it an option that would take care of this.

diff --git a/src/cpp/ffi/ffi_api.cpp b/src/cpp/ffi/ffi_api.cpp index 9e44f72..e6a388f 100644 --- a/src/cpp/ffi/ffi_api.cpp +++ b/src/cpp/ffi/ffi_api.cpp @@ -47,14 +47,14 @@ void ffi_api_set_arg(string s) { while (space < (int)s.size() && s[space] != ' ') space++; if (space >= (int)s.size()) {

  • argv[1] = strdup(s.c_str());
  • argv[1] = __builtin_strdup(s.c_str()); argv[2] = 0; argc = 2; } else { scopy = s; scopy[space] = 0;
  • argv[1] = strdup(scopy.c_str());
  • argv[2] = strdup(scopy.c_str() + space + 1);
  • argv[1] = __builtin_strdup(scopy.c_str());
  • argv[2] = __builtin_strdup(scopy.c_str() + space + 1); argv[3] = 0; argc = 3; } diff --git a/src/cpp/god.cpp b/src/cpp/god.cpp index 85a60bb..4bcbf1c 100644 --- a/src/cpp/god.cpp +++ b/src/cpp/god.cpp @@ -46,7 +46,7 @@ void dotwobitgod(puzdef &pd) { checkv = (checkv & 0x5555555555555555LL) & ((checkv >> 1) & 0x5555555555555555LL);

    ifdef HAVE_FFSLL

  • for (int smi = ffsll(checkv); checkv; smi = ffsll(checkv)) {
  • for (int smi = __builtin_ffsll(checkv); checkv; smi = __builtin_ffsll(checkv)) {

    else

     for (int smi = 1; checkv; smi++) {
       if (0 == ((checkv >> (smi - 1)) & 1))

    @@ -78,7 +78,7 @@ void dotwobitgod(puzdef &pd) { checkv = (checkv & 0x5555555555555555LL) & ((checkv >> 1) & 0x5555555555555555LL);

    ifdef HAVE_FFSLL

  • for (int smi = ffsll(checkv); checkv; smi = ffsll(checkv)) {
  • for (int smi = __builtin_ffsll(checkv); checkv; smi = __builtin_ffsll(checkv)) {

    else

     for (int smi = 1; checkv; smi++) {
       if (0 == ((checkv >> (smi - 1)) & 1))

    diff --git a/src/cpp/puzdef.h b/src/cpp/puzdef.h index a242dbf..94cd2ba 100644 --- a/src/cpp/puzdef.h +++ b/src/cpp/puzdef.h @@ -232,12 +232,12 @@ struct puzdef { for (int o = 1; o < setdefs[0].size; o++) { if ((r & (r - 1)) == 0) return r;

  • int t = ffsll(r) - 1;
  • int t = __builtin_ffsll(r) - 1; ull r2 = 1LL << t; int rv = rotinvmap[t].dat[bp[rotgroup[t].pos.dat[o]]]; r &= ~(1LL << t); while (r) {
  • int m = ffsll(r) - 1;
  • int m = __builtin_ffsll(r) - 1; r &= ~(1LL << m); t = rotinvmap[m].dat[bp[rotgroup[m].pos.dat[o]]]; if (t < rv) { diff --git a/src/cpp/rotations.cpp b/src/cpp/rotations.cpp index 77ca971..d57239c 100644 --- a/src/cpp/rotations.cpp +++ b/src/cpp/rotations.cpp @@ -295,11 +295,11 @@ int slowmodm2(const puzdef &pd, const setval p1, setval p2) { int cnt = 1; if (pd.rotgroup.size() <= 64) { ull lobits = pd.lowsymmbits(p1);
  • int g = ffsll(lobits) - 1;
  • int g = __builtin_ffsll(lobits) - 1; pd.mul3(pd.rotinvmap[g], p1, pd.rotgroup[g].pos, p2); lobits &= ~(1LL << g); while (lobits) {
  • g = ffsll(lobits) - 1;
  • g = __builtin_ffsll(lobits) - 1; lobits &= ~(1LL << g); int t = pd.mulcmp3(pd.rotinvmap[g], p1, pd.rotgroup[g].pos, p2); if (t <= 0) {

— Reply to this email directly, view it on GitHub https://github.com/cubing/twsearch/issues/41, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLSZWLBLANQMN632PLWLYDPXGTAVCNFSM6AAAAAA7DQNNP6VHI2DSMVQWIX3LMV43ASLTON2WKOZRHE4DIMZYHE4DGNY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

DougCube commented 1 year ago

Windows 10 / MSYS2 / MINGW64

$ g++ --version
g++ (GCC) 13.2.0
rokicki commented 1 year ago

Can you try now? I added the "missing" include files to provide the definitions for the functions, and tested on MacOS and Linux. Let me know if Windows gcc/g++ is happy with the changes.

Thanks!

DougCube commented 1 year ago

I just did 'git stash', 'git pull', and 'make build' but still hit same issue:

src/cpp/puzdef.h:236:15: error: ‘ffsll’ was not declared in this scope
  236 |       int t = ffsll(r) - 1;
      |               ^~~~~
make: *** [Makefile:110: build/cpp/antipode.o] Error 1
rokicki commented 1 year ago

I just tried spinning up a Windows machine on Azure, installed gcc (mingw-w64 13.2.0.0) and git, cloned, and built without any issues.

This is my gcc; can you show me yours?

C:\Users\rokicki\twsearch>g++ -v Using built-in specs. COLLECT_GCC=g++ COLLECT_LTO_WRAPPER=C:/Users/rokicki/w64devkit-1.20.0/w64devkit/bin/../libexec/gcc/x86_64-w64-mingw32/13.2.0/lto-wrapper.exe Target: x86_64-w64-mingw32 Configured with: /gcc-13.2.0/configure --prefix=/w64devkit --with-sysroot=/w64devkit/x86_64-w64-mingw32 --with-native-system-header-dir=/include --target=x86_64-w64-mingw32 --host=x86_64-w64-mingw32 --enable-static --disable-shared --with-pic --with-gmp-include=/deps/include --with-gmp-lib=/deps/lib --with-mpc-include=/deps/include --with-mpc-lib=/deps/lib --with-mpfr-include=/deps/include --with-mpfr-lib=/deps/lib --enable-languages=c,c++ --enable-libgomp --enable-threads=posix --enable-version-specific-runtime-libs --disable-dependency-tracking --disable-multilib --disable-nls --disable-win32-registry --enable-mingw-wildcard CFLAGS_FOR_TARGET=-Os CXXFLAGS_FOR_TARGET=-Os LDFLAGS_FOR_TARGET=-s CFLAGS=-Os CXXFLAGS=-Os LDFLAGS=-s Thread model: posix Supported LTO compression algorithms: zlib gcc version 13.2.0 (GCC)

The fix (made yesterday) to get ffsll and the other builtins was to include

and you should see that in puzdef.h and a few other places. I'm going to leave my Windows VM up for a day or two to help further debug this. But I don't see any issue here on my end. On Fri, Nov 10, 2023 at 7:50 AM DougCube ***@***.***> wrote: > I just did 'git stash', 'git pull', and 'make build' but still hit same > issue: > > src/cpp/puzdef.h:236:15: error: ‘ffsll’ was not declared in this scope > 236 | int t = ffsll(r) - 1; > | ^~~~~ > make: *** [Makefile:110: build/cpp/antipode.o] Error 1 > > — > Reply to this email directly, view it on GitHub > , > or unsubscribe > > . > You are receiving this because you commented.Message ID: > ***@***.***> > -- - https://cube20.org/ - https://golly.sf.net/ - https://alpha.twizzle.net/ / -
DougCube commented 1 year ago
/w/twsearch$ g++ -v
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-msys/13.2.0/lto-wrapper.exe
Target: x86_64-pc-msys
Configured with: /c/S/B/src/gcc-13.2.0/configure --build=x86_64-pc-msys --prefix=/usr --libexecdir=/usr/lib --enable-bootstrap --enable-shared --enable-shared-libgcc --enable-static --enable-version-specific-runtime-libs --with-arch=nocona --with-tune=generic --disable-multilib --enable-__cxa_atexit --with-dwarf2 --enable-languages=c,c++,lto --enable-graphite --enable-threads=posix --enable-libatomic --enable-libgomp --disable-libitm --enable-libquadmath --enable-libquadmath-support --disable-libssp --disable-win32-registry --disable-symvers --with-gnu-ld --with-gnu-as --disable-isl-version-check --enable-checking=release --without-libiconv-prefix --without-libintl-prefix --with-system-zlib --enable-linker-build-id --enable-libstdcxx-filesystem-ts
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 13.2.0 (GCC)
DougCube commented 1 year ago

I'm comfortable enough with GIT and GitHub. I first looked at your commit before I pulled. Checking that file now, I do see the recent changes you made.

rokicki commented 1 year ago

If you look at the top of util.h you'll see that we actually provide a definition for ffsll on Windows.

Is it possible your gcc is for some reason not defining the standard _WIN64 preprocessor symbol? I can't imagine why this would not work . . .

On Fri, Nov 10, 2023 at 10:16 AM DougCube @.***> wrote:

I'm comfortable enough with GIT and GitHub. I first looked at your commit before I pulled. Checking that file now, I do see the recent changes you made.

— Reply to this email directly, view it on GitHub https://github.com/cubing/twsearch/issues/41#issuecomment-1806198689, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLS445AD2WUFEJQJICF3YDZVOLAVCNFSM6AAAAAA7DQNNP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBWGE4TQNRYHE . You are receiving this because you commented.Message ID: @.***>

--

DougCube commented 1 year ago

I had an idea that perhaps the Makefile wasn't capturing some dependency when I tried it out with incremental-build. So I ran a fresh Git clone and tried building it totally clean, BUT same error.

/w/twsearch_2$ make build
mkdir -p build/cpp/ffi/
g++ -I./src/cpp/cityhash/src -c -O3 -Warray-bounds -Wextra -Wall -pedantic -std=c++17 -g -Wsign-compare -DTWSEARCH_VERSION=v0.6.4-59-ge03b5b0 -DUSE_PTHREADS src/cpp/antipode.cpp -o build/cpp/antipode.o
In file included from src/cpp/antipode.h:2,
                 from src/cpp/antipode.cpp:1:
src/cpp/puzdef.h: In member function ‘ull puzdef::lowsymmbits(setval) const’:
src/cpp/puzdef.h:236:15: error: ‘ffsll’ was not declared in this scope
  236 |       int t = ffsll(r) - 1;
      |               ^~~~~
make: *** [Makefile:110: build/cpp/antipode.o] Error 1
DougCube commented 1 year ago

You can probably replicate the problem on your end if you installed MSYS2 platform like I'm using.

rokicki commented 1 year ago

If you can find an appropriate preprocessor symbol that MSYS2 defines, and perhaps appropriate function definitions to add to util.h to provide support for MSYS2 compilation on Windows, I'd be happy to add that.

I do notice that I am using a couple of builtin_ functions already (__builtin_popcountll in one place, builtin_prefetch in another, __builtin_bswap64 in a third) but I consider these bugs and I need to fix this, moving system dependencies such as these into util.h. The code did, and should (but likely no longer does) build under MSVC, and I'd like to move towards a portable and sustainable codebase rather than introduce gcc'isms if we can avoid it.

I don't have a Windows machine and prefer to stay away from same, but will work on fixing the MSVC environment in the near future. But msys2 (a third Windows environment, distinct from the standard ming64/glibc and the more popular MSVC) is just one environment too many for me to mess with on Windows.

On Fri, Nov 10, 2023 at 12:20 PM DougCube @.***> wrote:

You can probably replicate the problem on your end if you installed MSYS2 platform like I'm using.

— Reply to this email directly, view it on GitHub https://github.com/cubing/twsearch/issues/41#issuecomment-1806383368, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLS2B5LWPMV2H4Q4EIDTYD2ECBAVCNFSM6AAAAAA7DQNNP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBWGM4DGMZWHA . You are receiving this because you commented.Message ID: @.***>

--

DougCube commented 1 year ago

> If you can find an appropriate preprocessor symbol that MSYS2 defines, and perhaps appropriate function definitions to add to util.h to provide support for MSYS2 compilation on Windows, I'd be happy to add that.

I just spent 15-20 minutes looking and debugging this issue but couldn't figure it out. AFAIK, it IS supposed to be in strings.h

DougCube commented 1 year ago

_> I do notice that I am using a couple of __builtin_ functions already (__builtin_popcountll in one place, __builtin_prefetch in another, __builtin_bswap64 in a third) but I consider these bugs and I need to fix this, moving system dependencies such as these into util.h._

Yes, I noticed that when I first hit the issue and I was going to mention it.

DougCube commented 1 year ago

I found a fix. In MSYS2, I ran pacman -S mingw-w64-x86_64-gcc to get and switch to the MINGW64 build of GCC instead of the MSYS2 one. It even automatically adjusted the PATH for me. I assumed I was already using that build of GCC before. I think it was due to the order I installed packages.

Can probably close this issue now.

rokicki commented 1 year ago

Thanks! Glad to have this one shut. If we ever get a Windows developer on the team perhaps we'll have a better Windows story.

On Fri, Nov 10, 2023 at 8:38 PM DougCube @.***> wrote:

I found a fix. In MSYS2, I ran pacman -S mingw-w64-x86_64-gcc to get and switch to the MINGW64 build of GCC instead of the MSYS2 one. It even automatically adjusted the PATH for me. I assumed I was already using that build of GCC before. I think it was due to the order I installed packages.

Can probably close this issue now.

— Reply to this email directly, view it on GitHub https://github.com/cubing/twsearch/issues/41#issuecomment-1806676333, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLS7PBRXF5LAIMNVZMCLYD36M3AVCNFSM6AAAAAA7DQNNP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBWGY3TMMZTGM . You are receiving this because you commented.Message ID: @.***>

--

rokicki commented 1 year ago

We will not be supporting MSYS2 builds unless we get a Windows developer. Closing this issue.