On zarch don't produce objects from assembler with a writable stack s…

[e4t]

…ection

On z-series, the current version of the GNU toolchain produces warnings such as:

/usr/lib64/gcc/[...]/s390x-suse-linux/bin/ld: warning: ztrmm_kernel_RC_Z14.o: missing .note.GNU-stack section implies
executable stack
/usr/lib64/[...]/s390x-suse-linux/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker

To prevent this message and make sure we are future proof, add

	.section        .note.GNU-stack,"",@progbits

[martin-frbg]

Yep, the message is annoying, but I'm still unsure if the .note would be compatible with non-GNU tools (?)

[e4t]

Yep, the message is annoying, but I'm still unsure if the .note would be compatible with non-GNU tools (?)

Yes, this is the big question. For z it may not be relevant if gcc was the only compiler supported. I hope to get clarification from IBM.
However, the toolchain team @ SUSE tells me that the assembler marks the stack executable by default - even on empty files (see cpuid.S on z). So I wonder why these messages do not appear on other platforms.
Presently, I'm trying to rely on the expertise of our toolchain for a recommendation how to handle this in a most compatible manner.
Another option would be to use the command line option -Wa,--noexecstack -Wl,-z,noexecstack - but again, these may be unknown to non-GNU compilers.

[martin-frbg]

The message is most prominent during the compilation of the getarch tool (on basically any platform with a new enough version of GNU binutils), but by all accounts it is harmless. If it really mattered, I guess we would need to have the c_check script query the assembler identity and version...

[Andreas-Krebbel]

Indeed, this needs fixing. Looking at how the other targets do it, we might want to define EPILOGUE to cover that part?
Like this perhaps (untested)?

diff --git a/common_zarch.h b/common_zarch.h
index 7911f11ae..54719d911 100644
--- a/common_zarch.h
+++ b/common_zarch.h
@@ -105,7 +105,9 @@ static inline int blas_quickdivide(blasint x, blasint y){
 REALNAME:
  
 
-#define EPILOGUE
+#define EPILOGUE \
+        .size   REALNAME, .-REALNAME; \
+        .section .note.GNU-stack,"",@progbits
 
 #define PROFCODE
 
diff --git a/kernel/zarch/ctrmm4x4V.S b/kernel/zarch/ctrmm4x4V.S
index 123f2ead0..73f023cb4 100644
--- a/kernel/zarch/ctrmm4x4V.S
+++ b/kernel/zarch/ctrmm4x4V.S
@@ -723,11 +723,4 @@ br %r14
 
 
 
-
-
-
-
-
-
-
-
+EPILOGUE
diff --git a/kernel/zarch/gemm8x4V.S b/kernel/zarch/gemm8x4V.S
index 633e60ea6..09d3e4268 100644
--- a/kernel/zarch/gemm8x4V.S
+++ b/kernel/zarch/gemm8x4V.S
@@ -609,3 +609,4 @@ br %r14
 
 
 
+EPILOGUE
diff --git a/kernel/zarch/strmm8x4V.S b/kernel/zarch/strmm8x4V.S
index e34a7a05a..b6cdca20b 100644
--- a/kernel/zarch/strmm8x4V.S
+++ b/kernel/zarch/strmm8x4V.S
@@ -853,3 +853,4 @@ br %r14
 
 
 
+EPILOGUE
diff --git a/kernel/zarch/trmm8x4V.S b/kernel/zarch/trmm8x4V.S
index 4da113ff3..4427e5f8d 100644
--- a/kernel/zarch/trmm8x4V.S
+++ b/kernel/zarch/trmm8x4V.S
@@ -867,8 +867,4 @@ br %r14
 .end
 
 
-
-
-
-
-
+EPILOGUE
diff --git a/kernel/zarch/ztrmm4x4V.S b/kernel/zarch/ztrmm4x4V.S
index 6fd7f2509..0e00cfeed 100644
--- a/kernel/zarch/ztrmm4x4V.S
+++ b/kernel/zarch/ztrmm4x4V.S
@@ -721,12 +721,7 @@ ld %f12,152(%r15)
 br %r14
 .end
 
-
-
-
-
-
-
+EPILOGUE
´´

[e4t]

The message is most prominent during the compilation of the getarch tool (on basically any platform with a new enough version of GNU binutils), but by all accounts it is harmless. If it really mattered, I guess we would need to have the c_check script query the assembler identity and version...

Yes, it's harmless, though annoying and it is not clear what the message "This behaviour is deprecated and will be removed in a future version of the linker" implies - is it just that the stack will no longer be executable and an application relying on it will core dump, or is it that linking will fail?
In the former case, we'd be safe.
For libopenblas it is more relevant as applications linking against it would potentially have an executable stack. This is seen as a potential security risk.
At SUSE, post build check scripts look for this, now and fail the build if packages contain binaries with executable stacks. I stumbled over this when I created a subpackage containing the openblas test suite (which is statically linked).

[Andreas-Krebbel]

Yep, the message is annoying, but I'm still unsure if the .note would be compatible with non-GNU tools (?)

Yes, this is the big question. For z it may not be relevant if gcc was the only compiler supported. I hope to get clarification from IBM. However, the toolchain team @ SUSE tells me that the assembler marks the stack executable by default - even on empty files (see cpuid.S on z). So I wonder why these messages do not appear on other platforms. Presently, I'm trying to rely on the expertise of our toolchain for a recommendation how to handle this in a most compatible manner. Another option would be to use the command line option -Wa,--noexecstack -Wl,-z,noexecstack - but again, these may be unknown to non-GNU compilers.

We support LLVM's assembler as well as Binutils. Both can handle the .note pseudo command. I see other targets gate the definition of EPILOGUE with a check for OS_LINUX. We should do the same then I think.

[martin-frbg]

Yes, that would (probably) work.

#if defined(__linux__) && defined(__ELF__)
#define GNUSTACK .section .note.GNU-stack,"",@progbits
#else
#define GNUSTACK
#endif

#define EPILOGUE \
        .size    REALNAME, .-REALNAME; \
        GNUSTACK

#endif

(this has me wondering about arm64 now, which would be about the last target arch without such an epilogue - though I don't recall seeing the assembler warning there, except for the getarch tool as mentioned)

[e4t]

Also, zarch doesn't use an EPILOG at all. I will check an aarch64 build tomorrow.

[martin-frbg]

Also, zarch doesn't use an EPILOG at all. I will check an aarch64 build tomorrow.

That in itself is (probably) no problem - some other platforms have empty EPILOGs, and AFAIK there's nothing wrong with an epilog consisting only of a .note
Actually I do not get any assembler/linker warnings on Debian/aarch64 in the GCC Compile Farm, even if I set LDFLAGS to --warn-execstack in case it is switched off by default. (Not sure if I'm missing something fundamental about the platform, and I'm very tired - but if I understand correctly, LLVM (for example) generates epilogues with the .note line on arm64 same as for x86_64)

[Andreas-Krebbel]

Also, zarch doesn't use an EPILOG at all. I will check an aarch64 build tomorrow.

That in itself is (probably) no problem - some other platforms have empty EPILOGs, and AFAIK there's nothing wrong with an epilog consisting only of a .note Actually I do not get any assembler/linker warnings on Debian/aarch64 in the GCC Compile Farm, even if I set LDFLAGS to --warn-execstack in case it is switched off by default. (Not sure if I'm missing something fundamental about the platform, and I'm very tired - but if I understand correctly, LLVM (for example) generates epilogues with the .note line on arm64 same as for x86_64)

I could reproduce it. It requires pretty recent binutils and you probably also have to build with DYNAMIC_ARCH=1 TARGET=ZARCH_GENERIC to force the .S files to be built into separate .o's.

[Andreas-Krebbel]

@e4t I have a tested patch now which defines EPILOGUE as in my comment above. Btw. adding .size also gives the asm defined functions a proper size in the symbol table:

11: 0000000000000000  8492 FUNC    GLOBAL DEFAULT    1 ztrmm_kernel_RC_Z13

This used be just 0 before. I could open another PR with it or would you rather want to continue here with your change?

[e4t]

@e4t I have a tested patch now which defines EPILOGUE as in my comment above. Btw. adding .size also gives the asm defined functions a proper size in the symbol table:

11: 0000000000000000  8492 FUNC    GLOBAL DEFAULT    1 ztrmm_kernel_RC_Z13

This used be just 0 before. I could open another PR with it or would you rather want to continue here with your change?

I've updated my patch as well (and split it in two: one for zarch and another one for getarc/cpuid.S. I can confirm that the messages are gone. I can go and re-push once the ppc64le build results are in.

[e4t]

Yep, the message is annoying, but I'm still unsure if the .note would be compatible with non-GNU tools (?)

Yes, this is the big question. For z it may not be relevant if gcc was the only compiler supported. I hope to get clarification from IBM.
However, the toolchain team @ SUSE tells me that the assembler marks the stack executable by default - even on empty files (see cpuid.S on z). So I wonder why these messages do not appear on other platforms.
Presently, I'm trying to rely on the expertise of our toolchain for a recommendation how to handle this in a most compatible manner.
Another option would be to use the command line option -Wa,--noexecstack -Wl,-z,noexecstack - but again, these may be unknown to non-GNU compilers.

[e4t]

It seems like there is a problem with the test suite. I've been closing and reopening this 7 times, now, to restart the test suite. Each time, it keeps failing in different tests which are obviously unrelated to the changes made.

[martin-frbg]

Umm, what's with the random closing and reopening now, if I may ask ? I can rerun individual CI jobs if necessary, if you can't

[martin-frbg]

there's no point in doing this when the failures are obviously unrelated - sometimes jobs encounter network errors when loading prerequisites, or (especially with Azure) they get scheduled on very slow machines where builds tend to time out.
rerunning everything just to make it look neat only burns cpu time (and in some cases costs the project actual money)

[e4t]

Umm, what's with the random closing and reopening now, if I may ask ? I can rerun individual CI jobs if necessary, if you can't

@martin-frbg Hrm, I don't see the widget that would allow me to rerun individual CI jobs - If this works for you, please do!

[e4t]

there's no point in doing this when the failures are obviously unrelated - sometimes jobs encounter network errors when loading prerequisites, or (especially with Azure) they get scheduled on very slow machines where builds tend to time out. rerunning everything just to make it look neat only burns cpu time (and in some cases costs the project actual money)

I do realize this, but I don't seem to have permissions to rerun individual CI jobs. Closing and reopening used to be the only way to get unstable tests to pass eventually, before partial re-run feature has been implemented.
Oh, and if the failures were related to the changes I'd probably have a look at the changes first before restarting anything.

BTW: This page describes the widget to rerun the failed jobs I was referring to (rotating arrows). Maybe there's another way I don't know about.