-
Notifications
You must be signed in to change notification settings - Fork 981
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
In gcc __powerpc64__ not __ppc64__ defines the PPC64 architecture #67
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
Thanks @grooverdan, this fixes the issue for me. |
reminder - quick compatible with clang fix for gcc users. |
Can someone please merge this patch? It's an obvious fix and over a year old. |
@googlebot rescan |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
happy bug day to you 🎂 |
@grooverdan I tried to find some docs around this, but everything I found states that both powerpc64 and PPC64 work. I also put together https://godbolt.org/z/cAf6BT which seems to confirm this theory. Docs: |
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.
Sorry it took so long to look into this. I'll approve the PR with the change below.
snappy-stubs-internal.h
Outdated
@@ -60,7 +60,7 @@ | |||
// Enable 64-bit optimized versions of some routines. | |||
#define ARCH_K8 1 | |||
|
|||
#elif defined(__ppc64__) | |||
#elif defined(__powerpc64__) |
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.
Can you please do #elif defined(__ppc__) || defined(__PPC__)
I think this works as well, and matches what we have in other parts of the codebase, for example: https://github.com/abseil/abseil-cpp/blob/38b704384cd2f17590b3922b97744be0b43622c9/absl/debugging/internal/stacktrace_config.h
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.
sorry it took so long. Lost track of this. Suggestion works for me, though will probably defined on ppc32 however it the stubs header is also covered by HAVE_BUILTIN_CTZ (compile checked), and FindMatchLength falls back to portable version.
A test of this: https://travis-ci.org/grooverdan/snappy/builds/600582620
Use 64-Bit ELF V2 ABI Specification for Power Architecture define __PPC__ (along with clang defined __ppc__) as authorative source for Power architecture. This enables gcc and other architecture compliant compilers to include Power Architecture optimized extensions. ref: http://openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655243_75216.html
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Co-Authored-By: Alexander Grund <Flamefire@users.noreply.github.com>
I'm really sorry, I completely lost track of this 😢 Based on new data, I plan to fix this problem using the approach here: pwnall@a9955e5 |
Corrects 18488d6 and still maintains
clang compatibility (like the #27 originally).
Compiler tests:
$ uname -m
ppc64le
$ cat /tmp/x.c
$ gcc -c /tmp/x.c
/tmp/x.c:3:2: warning: #warning powerpc64 exists [-Wcpp]
#warning powerpc64 exists
^~~~~~~
/tmp/x.c:11:2: error: #error ppc64 not defined
#error ppc64 not defined
^~~~~
$ clang /tmp/x.c
/tmp/x.c:3:2: warning: powerpc64 exists [-W#warnings]
^
/tmp/x.c:9:2: warning: ppc64 exists [-W#warnings]
^
2 warnings generated.
I should be on the CLA list already.