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.

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

Added gcc-10.2.0 patches #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

saidganim
Copy link

No description provided.

@richfelker
Copy link
Owner

It looks like a good many of the patches from 9.x have been dropped. Are they all upstream? I notice the s390x arch support patch is still there but m68k was dropped, so was one of these upstreamed but not the other? And in particular, omitting the the invalid tls model and gth-weak patches would produce silent breakage if they're not upstream.

@saidganim
Copy link
Author

@richfelker hey, yes, I didn't include only those patches, which are in upstream. So it seems to be safe :)

@saidganim
Copy link
Author

@richfelker Are we merging this PR ?

Copy link

@TOCK-Chiu TOCK-Chiu left a comment

Choose a reason for hiding this comment

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

Just a minor question regarding 0009-static-pie.diff.

Comment on lines 41 to 43
"%{!static:%{fvtable-verify=none:%s; \
fvtable-verify=preinit:vtv_end_preinit.o%s; \
fvtable-verify=std:vtv_end.o%s}} \

Choose a reason for hiding this comment

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

According to the commit that changed this line between GCC 9 and 10, I think we should exclude static-pie as well, no?

For example (borrowed from Szabolcs Nagy):

Suggested change
"%{!static:%{fvtable-verify=none:%s; \
fvtable-verify=preinit:vtv_end_preinit.o%s; \
fvtable-verify=std:vtv_end.o%s}} \
"%{static|static-pie:; \
fvtable-verify=none:%s; \
fvtable-verify=preinit:vtv_end_preinit.o%s; \
fvtable-verify=std:vtv_end.o%s} \

Copy link
Author

@saidganim saidganim Sep 25, 2020

Choose a reason for hiding this comment

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

Good point. Thank you. Seems, you are right I think we should include exclude it, otherwise compiling static-pie binary should give 'multiple definition of ...' linking error. To be honest, I am not sure whether it was a good idea in gcc10 to make tentative variables go into bss .. (I want to be wrong).

Copy link
Author

Choose a reason for hiding this comment

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

@richfelker any thoughts?

Copy link

@TOCK-Chiu TOCK-Chiu left a comment

Choose a reason for hiding this comment

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

Just some more questions.

Comment on lines 1 to 21
diff --git a/libatomic/testsuite/lib/libatomic.exp b/libatomic/testsuite/lib/libatomic.exp
index 38f3e5673e2..02ec2e8b48a 100644
--- a/libatomic/testsuite/lib/libatomic.exp
+++ b/libatomic/testsuite/lib/libatomic.exp
@@ -77,6 +77,7 @@ proc libatomic_init { args } {
global ALWAYS_CFLAGS
global CFLAGS
global TOOL_EXECUTABLE TOOL_OPTIONS
+ global BUILD_CC
global GCC_UNDER_TEST
global TESTING_IN_BUILD_TREE
global target_triplet
@@ -92,6 +93,8 @@ proc libatomic_init { args } {
if ![info exists GCC_UNDER_TEST] then {
if [info exists TOOL_EXECUTABLE] {
set GCC_UNDER_TEST $TOOL_EXECUTABLE
+ } elseif [info exists BUILD_CC] {
+ set GCC_UNDER_TEST $BUILD_CC
} else {
set GCC_UNDER_TEST "[find_gcc]"
}

Choose a reason for hiding this comment

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

This might have been fixed upstream in another way?

Comment on lines 1 to 62
diff --git a/libgomp/testsuite/Makefile.am b/libgomp/testsuite/Makefile.am
index 655a413c160..916e9d30f15 100644
--- a/libgomp/testsuite/Makefile.am
+++ b/libgomp/testsuite/Makefile.am
@@ -14,6 +14,13 @@ RUNTESTDEFAULTFLAGS = --tool $$tool --srcdir $$srcdir

EXTRA_DEJAGNU_SITE_CONFIG = libgomp-site-extra.exp

+EXTRA_DEJAGNU_SITE_CONFIG = extra.exp
+
+extra.exp:
+ echo 'set BUILD_CC "$(CC)"' > [email protected]
+ mv [email protected] $@
+
+
# Instead of directly in ../testsuite/libgomp-test-support.exp.in, the
# following variables have to be "routed through" this Makefile, for expansion
# of the several (Makefile) variables used therein.
diff --git a/libgomp/testsuite/Makefile.in b/libgomp/testsuite/Makefile.in
index 52aa6c5fbc9..617da8ddbf2 100644
--- a/libgomp/testsuite/Makefile.in
+++ b/libgomp/testsuite/Makefile.in
@@ -311,6 +311,7 @@ _RUNTEST = $(shell if test -f $(top_srcdir)/../dejagnu/runtest; then \
echo $(top_srcdir)/../dejagnu/runtest; else echo runtest; fi)

RUNTESTDEFAULTFLAGS = --tool $$tool --srcdir $$srcdir
+EXTRA_DEJAGNU_SITE_CONFIG = extra.exp
EXTRA_DEJAGNU_SITE_CONFIG = libgomp-site-extra.exp
all: all-am

@@ -475,6 +476,10 @@ uninstall-am:
.PRECIOUS: Makefile


+extra.exp:
+ echo 'set BUILD_CC "$(CC)"' > [email protected]
+ mv [email protected] $@
+
# Instead of directly in ../testsuite/libgomp-test-support.exp.in, the
# following variables have to be "routed through" this Makefile, for expansion
# of the several (Makefile) variables used therein.
diff --git a/libgomp/testsuite/lib/libgomp.exp b/libgomp/testsuite/lib/libgomp.exp
index ee5f0e57b19..183081b5802 100644
--- a/libgomp/testsuite/lib/libgomp.exp
+++ b/libgomp/testsuite/lib/libgomp.exp
@@ -68,6 +68,7 @@ proc libgomp_init { args } {
global ALWAYS_CFLAGS
global CFLAGS
global TOOL_EXECUTABLE TOOL_OPTIONS
+ global BUILD_CC
global GCC_UNDER_TEST
global TESTING_IN_BUILD_TREE
global target_triplet
@@ -90,6 +91,8 @@ proc libgomp_init { args } {
if ![info exists GCC_UNDER_TEST] then {
if [info exists TOOL_EXECUTABLE] {
set GCC_UNDER_TEST $TOOL_EXECUTABLE
+ } elseif [info exists BUILD_CC] {
+ set GCC_UNDER_TEST $BUILD_CC
} else {
set GCC_UNDER_TEST "[find_gcc]"
}

Choose a reason for hiding this comment

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

This might have been fixed upstream in another way, as well?

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I am generating patches for gcc10.2.0 anyway, I will update this PR

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your revision

Comment on lines 9 to 10
+#define MUSL_DYNAMIC_LINKER32 "/lib/ld-musl-s390.so.1"
+#define MUSL_DYNAMIC_LINKER64 "/lib/ld-musl-s390x.so.1"

Choose a reason for hiding this comment

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

This has been added upstream, along with another fix.

Copy link
Author

Choose a reason for hiding this comment

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

You are right.

@rofl0r
Copy link
Contributor

rofl0r commented Sep 29, 2020

i think it would be preferable to merge the patches done by szabolcs nagy, as he's a long-time contributor to both musl and gcc and certainly did a careful forward port of the missing patches.
note that he also provided most of the existing patch sets.

@richfelker
Copy link
Owner

@rofl0r Looking at https://port70.net/~nsz/musl/gcc-10.1.0/, it seems there are patches missing that are included in this set: s390x ldso, time64 futex support, and m68k sqrt, at least. It sounds like the test fixes might be fixed upstream and no longer needed though.

Also, the time64 futex patch needs a fix to be riscv32-ready. I've pushed some pending changes I had here that can be incorporated.

@saidganim saidganim changed the title Added gcc-10.1.0 patches Added gcc-10.2.0 patches Oct 1, 2020
@rofl0r
Copy link
Contributor

rofl0r commented Jul 7, 2021

superceded by #124 , this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants