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

Jump to bottom

Fix checking for existing builtin overflow functions #958

Merged
merged 3 commits into from
May 30, 2024
Merged

Fix checking for existing builtin overflow functions #958

merged 3 commits into from
May 30, 2024

Conversation

red1452
Copy link

@red1452 red1452 commented May 29, 2024 •
edited
Loading

Commit 1ca5486 added support of using builtin functions:

__builtin_add_overflow
__builtin_sub_overflow
__builtin_mul_overflow

Check for existing these functions was added to the configure script. But it did not work, because gcc printed warning instead of error and returned success code. To the confdefs.h macro #define HAVE__BUILTIN_OP_OVERFLOW 1 was printed and at the make step compiler tried to build sources with these builtin functions. It lead to errors:

In file included from arrayfuncs.c:21:0:
../../../../src/include/common/int.h: In function 'pg_add_s16_overflow':
../../../../src/include/common/int.h:32:2: error: implicit declaration of function '__builtin_add_overflow' [-Werror=implicit-function-declaration]
 return __builtin_add_overflow(a, b, result);
 ^
./../../../src/include/common/int.h: In function 'pg_sub_s16_overflow':
../../../../src/include/common/int.h:52:2: error: implicit declaration of function '__builtin_sub_overflow' [-Werror=implicit-function-declaration]
return __builtin_sub_overflow(a, b, result);
^
../../../../src/include/common/int.h: In function 'pg_mul_s16_overflow':
../../../../src/include/common/int.h:72:2: error: implicit declaration of function '__builtin_mul_overflow' [-Werror=implicit-function-declaration]
return __builtin_mul_overflow(a, b, result);
^
cc1: some warnings being treated as errors
make[5]: *** [arrayfuncs.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [adt-recursive] Error 2
make[3]: *** [utils-recursive] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [install-backend-recurse] Error 2
make[1]: *** [install-src-recurse] Error 2
make: *** [dist] Error 2
ERROR: process "/bin/sh -c bash /home/gpadmin/gpdb_src/concourse/scripts/compile_gpdb.bash" did not complete successfully: exit code: 2

At PostgreSQL there are commits, which fix the problem. These commits change AC_COMPILE_IFELSE to AC_LINK_IFELSE for the test program at the configure script, which should check for existing builtin functions.

This patch cherry-picks three commits from postgres:
commit 85abb5b
commit c04d35f
commit c6d21d5

DO not squash it.

Error at the unit test will be fixed at the other PR. This PR fixes problem at the configure script. It means, that only building should be success. It was checked at the pipeline.

@red1452 red1452 changed the base branch from 6.27.1-sync to adb-6.x May 29, 2024 07:58
@BenderArenadata
Copy link

Failed job Build for ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1448913

@BenderArenadata
Copy link

Failed job Build for x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1448912

@BenderArenadata
Copy link

Failed job Build for x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1449011

@BenderArenadata
Copy link

Failed job Build for ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1449013

@red1452 red1452 changed the base branch from adb-6.x to 6.27.1-sync May 29, 2024 14:23
@BenderArenadata
Copy link

Failed job Build for ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1451507

@BenderArenadata
Copy link

Failed job Build for x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1451506

@Stolb27 Stolb27 requested a review from a team May 29, 2024 14:35
Stolb27
Stolb27 requested changes May 29, 2024
View reviewed changes
Copy link
Collaborator

@Stolb27 Stolb27 left a comment

Choose a reason for hiding this comment

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

You mustn't modify configure file by hands. Modify configure.in and regenerate configure file, instead.

red1452 added a commit that referenced this pull request May 29, 2024
@red1452
Fix checking for existing builtin overflow functions (#958)
d548cff 
This patch fixes commit 1ca5486, which added
support of using builtin overflow functions: __builtin_add_overflow,
__builtin_sub_overflow, __builtin_mul_overflow. The configure script tries to
compile a small program with one of these functions. Compilation was without any
errors, because to correctly check gcc requires flag
-Werror=implicit-function-declaration. This flag is added later.

This patch moves this check after flag -Werror=implicit-function-declaration is
added to CFLAGS.
@red1452 red1452 changed the base branch from 6.27.1-sync to adb-6.x May 29, 2024 17:19
red1452 added a commit that referenced this pull request May 29, 2024
@red1452
Fix checking for existing builtin overflow functions (#958)
0fc169c 
This patch fixes commit 1ca5486, which added
support of using builtin overflow functions: __builtin_add_overflow,
__builtin_sub_overflow, __builtin_mul_overflow. The configure script tries to
compile a small program with one of these functions. Compilation was without any
errors, because to correctly check gcc requires flag
-Werror=implicit-function-declaration. This flag is added later.

This patch moves this check after flag -Werror=implicit-function-declaration is
added to CFLAGS.
@BenderArenadata
Copy link

Failed job Build for x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1451993

@BenderArenadata
Copy link

Failed job Build for ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1451994

@red1452 red1452 changed the base branch from adb-6.x to 6.27.1-sync May 30, 2024 06:50
@anarazel @red1452
Make PGAC_C_BUILTIN_OP_OVERFLOW link instead of just compiling.
fab1628 
Otherwise the detection can spuriously detect symbol as available,
because the compiler may just emits reference to non-existant symbol.

(cherry picked from commit 85abb5b)
@anarazel @red1452
Try to detect runtime unavailability of __builtin_mul_overflow(int64).
03bdf59 
On some systems the results of 64 bit __builtin_mul_overflow()
operations can be computed at compile time, but not at runtime. The
known cases are arm buildfar animals using clang where the runtime
operation is implemented in a unavailable function.

Try to avoid compile-time computation by using volatile arguments to
__builtin_mul_overflow(). In that case we hopefully will get a link
error when unavailable, similar to what buildfarm animals dangomushi
and gull are reporting.

Author: Andres Freund
Discussion: https://postgr.es/m/20171213213754.pydkyjs6bt2hvsdb@alap3.anarazel.de
(cherry picked from commit c04d35f)
@red1452 red1452 changed the base branch from 6.27.1-sync to adb-6.x May 30, 2024 11:23
@tglsfdc @red1452
Try harder to detect unavailability of __builtin_mul_overflow(int64).
9c32ad1 
Commit c04d35f didn't quite do the job here, because it still allowed
the compiler to deduce that the function call could be optimized away.
Prevent that by putting the arguments and results in global variables.

Discussion: https://postgr.es/m/20171213213754.pydkyjs6bt2hvsdb@alap3.anarazel.de
(cherry picked from commit c6d21d5)
@red1452 red1452 changed the base branch from adb-6.x to 6.27.1-sync May 30, 2024 11:40
@BenderArenadata
Copy link

Failed job Build for x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1455592

@BenderArenadata
Copy link

Failed job Build for ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/1455593

@Stolb27 Stolb27 merged commit 693cc8f into 6.27.1-sync May 30, 2024
3 of 5 checks passed
@Stolb27 Stolb27 deleted the ADBDEV-5711 branch May 30, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@InnerLife0 InnerLife0 InnerLife0 approved these changes

@Stolb27 Stolb27 Awaiting requested review from Stolb27

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@red1452 @BenderArenadata @Stolb27 @InnerLife0 @anarazel @tglsfdc