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

__mips__ is defined for both 32 and 64bit mips on Android #813

Closed
tomaz82 opened this Issue May 18, 2016 · 10 comments

Comments

Projects
None yet
2 participants
@tomaz82

tomaz82 commented May 18, 2016

I did this

Custom compile script for Android, compiling for mips64 fails due to CURL_SIZEOF_LONG not matching sizeof(long)

I expected the following

curlbuild.h line 532 defined(__mips__) is true for both 32 and 64 bit mips when compiling for Android, changing it to (defined(__mips__) && !defined(__LP64__)) fixes it.

curl/libcurl version

version 7.48

OS version

Compiling for Android in Windows 7

@bagder bagder added the build label May 18, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 18, 2016

Member

This has been mentioned before (here for example). That file (curlbuild.h as it appears in the distribution tarball) is really not intended to be used on systems that can run configure and generate its proper header, which should include most systems that can run on MIPS. When configure runs, it outputs a generated version for that particular system.

I could still see a value in fixing this so that maybe we can work on, long-term, move (back) toward not generating a system specific curlbuild.h at build time.

Can you suggest a full patch that you deem proper for this to build fine for you?

Member

bagder commented May 18, 2016

This has been mentioned before (here for example). That file (curlbuild.h as it appears in the distribution tarball) is really not intended to be used on systems that can run configure and generate its proper header, which should include most systems that can run on MIPS. When configure runs, it outputs a generated version for that particular system.

I could still see a value in fixing this so that maybe we can work on, long-term, move (back) toward not generating a system specific curlbuild.h at build time.

Can you suggest a full patch that you deem proper for this to build fine for you?

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 May 18, 2016

In understand that it's a generic header not meant to be used, but when building for Android from Windows using jni, there are no easy way to run configure

The fix I suggested that checks if __mips__ is defined AND that __LP64__ isn't defined is sufficient. I have that in my local version and can build libcurl for Android just fine on all 7 major architectures ( armeabi, armeabi-v7a, mips, x86, arm64-v8a, mips64 and x64 ).

tomaz82 commented May 18, 2016

In understand that it's a generic header not meant to be used, but when building for Android from Windows using jni, there are no easy way to run configure

The fix I suggested that checks if __mips__ is defined AND that __LP64__ isn't defined is sufficient. I have that in my local version and can build libcurl for Android just fine on all 7 major architectures ( armeabi, armeabi-v7a, mips, x86, arm64-v8a, mips64 and x64 ).

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 18, 2016

Member

And you really need LP64 checked there and not __LP64__ ? We check the latter symbol for the 64 bit case so I figure it would make it more consistent if we would check the same!

Member

bagder commented May 18, 2016

And you really need LP64 checked there and not __LP64__ ? We check the latter symbol for the 64 bit case so I figure it would make it more consistent if we would check the same!

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 May 18, 2016

Yeah I meant the __LP64__, the formatting in this issue tracker turned it into bold text.

tomaz82 commented May 18, 2016

Yeah I meant the __LP64__, the formatting in this issue tracker turned it into bold text.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 18, 2016

Member

Something like this?

diff --git a/include/curl/curlbuild.h.dist b/include/curl/curlbuild.h.dist
index 58323d0..ae95095 100644
--- a/include/curl/curlbuild.h.dist
+++ b/include/curl/curlbuild.h.dist
@@ -525,13 +525,13 @@
 /* ===================================== */
 /*    KEEP GENERIC GCC THE LAST ENTRY    */
 /* ===================================== */

 #elif defined(__GNUC__)
-#  if defined(__ILP32__) || \
+#  if !defined(__LP64__) && (defined(__ILP32__) || \
       defined(__i386__) || defined(__ppc__) || defined(__arm__) || \
-      defined(__sparc__) || defined(__mips__) || defined(__sh__)
+      defined(__sparc__) || defined(__mips__) || defined(__sh__))
 #    define CURL_SIZEOF_LONG           4
 #    define CURL_TYPEOF_CURL_OFF_T     long long
 #    define CURL_FORMAT_CURL_OFF_T     "lld"
 #    define CURL_FORMAT_CURL_OFF_TU    "llu"
 #    define CURL_FORMAT_OFF_T          "%lld"
Member

bagder commented May 18, 2016

Something like this?

diff --git a/include/curl/curlbuild.h.dist b/include/curl/curlbuild.h.dist
index 58323d0..ae95095 100644
--- a/include/curl/curlbuild.h.dist
+++ b/include/curl/curlbuild.h.dist
@@ -525,13 +525,13 @@
 /* ===================================== */
 /*    KEEP GENERIC GCC THE LAST ENTRY    */
 /* ===================================== */

 #elif defined(__GNUC__)
-#  if defined(__ILP32__) || \
+#  if !defined(__LP64__) && (defined(__ILP32__) || \
       defined(__i386__) || defined(__ppc__) || defined(__arm__) || \
-      defined(__sparc__) || defined(__mips__) || defined(__sh__)
+      defined(__sparc__) || defined(__mips__) || defined(__sh__))
 #    define CURL_SIZEOF_LONG           4
 #    define CURL_TYPEOF_CURL_OFF_T     long long
 #    define CURL_FORMAT_CURL_OFF_T     "lld"
 #    define CURL_FORMAT_CURL_OFF_TU    "llu"
 #    define CURL_FORMAT_OFF_T          "%lld"
@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 May 18, 2016

That might work I guess, I did it different, as mentioned in my initial bug report, trying to add a patch here but the formatting keeps messing it up.

tomaz82 commented May 18, 2016

That might work I guess, I did it different, as mentioned in my initial bug report, trying to add a patch here but the formatting keeps messing it up.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 May 18, 2016

diff -r 342d4b87724b thirdparty/curl-7.48.0/include/curl/curlbuild.h
--- a/thirdparty/curl-7.48.0/include/curl/curlbuild.h   Wed May 18 14:12:01 2016 +0200
+++ b/thirdparty/curl-7.48.0/include/curl/curlbuild.h   Wed May 18 14:12:29 2016 +0200
@@ -529,7 +529,7 @@
 #elif defined(__GNUC__)
 #  if defined(__ILP32__) || \
       defined(__i386__) || defined(__ppc__) || defined(__arm__) || \
-      defined(__sparc__) || defined(__mips__) || defined(__sh__)
+      defined(__sparc__) || (defined(__mips__) && !defined(__LP64__)) || defined(__sh__)
 #    define CURL_SIZEOF_LONG           4
 #    define CURL_TYPEOF_CURL_OFF_T     long long
 #    define CURL_FORMAT_CURL_OFF_T     "lld"

tomaz82 commented May 18, 2016

diff -r 342d4b87724b thirdparty/curl-7.48.0/include/curl/curlbuild.h
--- a/thirdparty/curl-7.48.0/include/curl/curlbuild.h   Wed May 18 14:12:01 2016 +0200
+++ b/thirdparty/curl-7.48.0/include/curl/curlbuild.h   Wed May 18 14:12:29 2016 +0200
@@ -529,7 +529,7 @@
 #elif defined(__GNUC__)
 #  if defined(__ILP32__) || \
       defined(__i386__) || defined(__ppc__) || defined(__arm__) || \
-      defined(__sparc__) || defined(__mips__) || defined(__sh__)
+      defined(__sparc__) || (defined(__mips__) && !defined(__LP64__)) || defined(__sh__)
 #    define CURL_SIZEOF_LONG           4
 #    define CURL_TYPEOF_CURL_OFF_T     long long
 #    define CURL_FORMAT_CURL_OFF_T     "lld"
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 18, 2016

Member

Right, I prepended that 64bit check because I figured it is such an important define that if set on any platform it implies 64bit.

Member

bagder commented May 18, 2016

Right, I prepended that 64bit check because I figured it is such an important define that if set on any platform it implies 64bit.

@tomaz82

This comment has been minimized.

Show comment
Hide comment
@tomaz82

tomaz82 May 18, 2016

That will work

tomaz82 commented May 18, 2016

That will work

@bagder bagder closed this in 63e1f06 May 18, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 18, 2016

Member

Thanks!

Member

bagder commented May 18, 2016

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.