Skip to content
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.

Sign up for GitHub

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

[gklib] Fix build error with Android triplet #37416

Closed
wants to merge 2 commits into from
Closed

[gklib] Fix build error with Android triplet #37416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2271e3
[gklib] Fix build error with clang 17
Cheney-W Mar 13, 2024
9667980
Modify patch
Cheney-W Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions ports/gklib/fix-error-in-clang17.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
diff --git a/error.c b/error.c
index e2a18cf..a42122b 100644
--- a/error.c
+++ b/error.c
@@ -15,6 +15,10 @@ This file contains functions dealing with error reporting and termination

#include <GKlib.h>

+#ifdef HAVE_EXECINFO_H
+ extern int backtrace(void **buffer, int size);
+ extern char **backtrace_symbols(void *const *buffer, int size);
+#endif
Comment on lines +9 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you know you have execinfo.h, and these functions are declared in execinfo.h, why don't you #include <execinfo.h>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've considered the situation you mentioned. In the issue I submitted to the upstream, I mentioned two possible solutions, one is to add #include <execinfo.h>, but I noticed that the upstream source already has the following inclusion relationship:
error.c have #include <GKlib.h>
GKlib.h have #include "gk_arch.h"
gk_arch.h have:

#ifdef HAVE_EXECINFO_H
#include <execinfo.h>
#endif

So, instead of directly including the header file, I opted for using extended function declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, these are just my personal thoughts. If you think it's better to directly include the header file, I'll adjust my PR accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, the fix is for android, not for clang-17?!
Then the tentative fix is there:
https://github.com/microsoft/vcpkg/pull/35851/files#diff-95c803e4c056c6f158a02350ea41a27b7c646b9571048daff3707d20be342f68

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. it needs API level 33.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've considered the situation you mentioned. In the issue I submitted to the upstream, I mentioned two possible solutions, one is to add #include <execinfo.h>, but I noticed that the upstream source already has the following inclusion relationship: error.c have #include <GKlib.h> GKlib.h have #include "gk_arch.h" gk_arch.h have:

#ifdef HAVE_EXECINFO_H
#include <execinfo.h>
#endif

So, instead of directly including the header file, I opted for using extended function declarations.

That suggests that HAVE_EXECINFO_H was off and therefore this patch doesn't do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error-prone code is also confined within #ifdef HAVE_EXECINFO_H,

#ifdef HAVE_EXECINFO_H
...
  size = backtrace(array, 10);
  strings = backtrace_symbols(array, size);
...
#endif

so when HAVE_EXECINFO_H is off, the error will not be triggered, and there is no need for patching work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR's title is misleading. It is not a clang 17 issue.

The truth is that on Android execinfo.h can be present (detected) but still nthe functions are not available until raising the API level.

And the fix proposed here fixes the static lib build, but users will hit the wall when they try to link executables.


/* These are the jmp_buf for the graceful exit in case of severe errors.
Multiple buffers are defined to allow for recursive invokation. */
2 changes: 1 addition & 1 deletion ports/gklib/fix-mingw.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ index 122e087..c8a322f 100644
#endif
#include <inttypes.h>
#include <sys/types.h>
+#ifndef _WIN32 # MinGW
+#ifndef _WIN32
#include <sys/resource.h>
+#endif
#include <sys/time.h>
Expand Down
1 change: 1 addition & 0 deletions ports/gklib/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ vcpkg_from_github(
PATCHES
build-fixes.patch
fix-mingw.patch
fix-error-in-clang17.patch
)

# Delete files that are workarounds for very old copies of msvc.
Expand Down
2 changes: 1 addition & 1 deletion ports/gklib/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "gklib",
"version-date": "2022-07-27",
"port-version": 2,
"port-version": 3,
"description": "General helper libraries for KarypisLab.",
"homepage": "https://github.com/KarypisLab/GKlib/",
"license": "Apache-2.0",
Expand Down
2 changes: 1 addition & 1 deletion versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3010,7 +3010,7 @@
},
"gklib": {
"baseline": "2022-07-27",
"port-version": 2
"port-version": 3
},
"gl2ps": {
"baseline": "1.4.2",
Expand Down
5 changes: 5 additions & 0 deletions versions/g-/gklib.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "54f660cd46f930ee5df78cb91188b52fbff38e34",
"version-date": "2022-07-27",
"port-version": 3
},
{
"git-tree": "7f9c6f0e711860da19899fbe78e027d0a5e8cfc1",
"version-date": "2022-07-27",
Expand Down