Bug#995393: fakeroot: FTBFS on ppc64el
Control: reopen 995393
Control: tags 995393 patch
Christoph Biedl wrote...
> As a workaround I propose a tactical __attribute__ to locally disable
> optimization as this issue has been blocking eatmydata for way too long,
> and performance loss should be neglectable.
So as always with a band-aid, it's wiser to address the underlying
problem than to work around it - this time fakeroot broke the glibc
autopkgtest ...
As jtrc27 pointed out in IRC, ppc64el is fairly picky when it comes to
va_arg handling, and next_openat needs a correct "..." parameter in the
prototype to have proper code generated.
As a surprise, it seems the original submitter had that in mind by
introducing a sixth column to wrapfunc.inp, although it's not used by
wrapwak (and not documented either, fix for that included).
My solution isn't nice as it hardcodes the va_arg handling into the
expansion, making it a special for wrapping the openat call. There's
always room for improvement B=)
Additionally, some headers are not necesarily re-generated - so add
an extra rm call in the clean target to enforce this.
Status: Passes on the porterbox, no compiler warnings/errors from
a prototype mismatch.
Christoph
From d30127a5b2a2f26ae2f6ddc8df077a548ec145e0 Mon Sep 17 00:00:00 2001
From: Christoph Biedl
Date: Thu, 30 Dec 2021 19:05:37 +0100
Subject: [PATCH] Fix segfault on ppc64el, take 2
---
debian/patches/fix-prototype-generation.patch | 53 +++
debian/patches/ppc64el-workaround.patch | 31 ---
debian/patches/series | 2 +-
debian/rules | 1 +
4 files changed, 55 insertions(+), 32 deletions(-)
create mode 100644 debian/patches/fix-prototype-generation.patch
delete mode 100644 debian/patches/ppc64el-workaround.patch
diff --git a/debian/patches/fix-prototype-generation.patch
b/debian/patches/fix-prototype-generation.patch
new file mode 100644
index 000..d364c5c
--- /dev/null
+++ b/debian/patches/fix-prototype-generation.patch
@@ -0,0 +1,53 @@
+Subject: Fix prototype generation for openat
+Author: Christoph Biedl
+Date: 2021-12-30
+Bug-Debian: https://bugs.debian.org/995393
+Forwarded: Yes (implicitely)
+
+As jtrc27 pointed out in IRC, ppc64el is fairly picky when it comes to
+va_arg handling, and next_openat needs a correct "..." parameter in the
+prototype to have proper code generated.
+
+--- a/wrapawk
b/wrapawk
+@@ -37,7 +37,25 @@
+ argtype=$3;
+ argname=$4;
+ MACRO=$5;
+- if(MACRO){
++ openat_extra=$6;
++ if(openat_extra){
++print " {(void(*))_" name ", \"" name "\"}," > structfile;
++print "extern " ret " (*next_" name ")" openat_extra ";" > headerfile;
++print ret " (*next_" name ")" openat_extra "=tmp_" name ";"> deffile;
++
++print ret " tmp_" name, openat_extra "{" > tmpffile;
++print " mode_t mode = 0;"> tmpffile;
++print " if (flags & O_CREAT) {"> tmpffile;
++print " va_list args;"> tmpffile;
++print " va_start(args, flags);"> tmpffile;
++print " mode = va_arg(args, int);"> tmpffile;
++print " va_end(args);"> tmpffile;
++print " }"> tmpffile;
++print " load_library_symbols();" > tmpffile;
++print " return next_" name, argname ";" > tmpffile;
++print "}" > tmpffile;
++print ""> tmpffile;
++ } else if(MACRO){
+ print " {(void(*))_" MACRO "_NOARG, " name "_QUOTE}," > structfile;
+ print "extern " ret " (*NEXT_" MACRO "_NOARG)" argtype ";" > headerfile;
+ print ret " (*NEXT_" MACRO "_NOARG)" argtype "=TMP_" MACRO ";"> deffile;
+--- a/wrapfunc.inp
b/wrapfunc.inp
+@@ -9,8 +9,10 @@
+ /**/*/
+ /* each line of this file lists 4 fields, seperated by a ";". */
+ /* The first field is the name of the wrapped function, then it's return */
+-/* value. After that come the function arguments with types, and the last */
++/* value. After that come the function arguments with types, and the fifth */
+ /* field contains the function arguments without types. */
++/* A sixth field is a special needed when wrapping the openat syscall.*/
++/* Otherwise it's like the third (function arguments with types). */
+ /**/
+
+ /* __*xstat are used on glibc systems instead of just *xstat. */
diff --git a/debian/patches/ppc64el-workaround.patch
b/debian/patches/ppc64el-workaround.patch
deleted file mode 100644
index 95b0ff0..000
--- a/debian/patches/ppc64el-workaround.patch
+++ /dev/null
@@ -1,31 +0,0 @@
-Subject: Work around segfault on ppc64el
-Author: Christoph Biedl
-Date: 2021-12-20
-Bug-Debian: https://bugs.debian.org/995393
-Forwarded: Yes
-
-For whatever reason the generated code segfaults on ppc64el when
-built with
Bug#995393: fakeroot: FTBFS on ppc64el
Control: tags 995393 patch
Frédéric Bonnard wrote...
> #ifdef HAVE_OPENAT
> +#if defined(__powerpc__) && defined(__powerpc64__) && __BYTE_ORDER__ ==
> __ORDER_LITTLE_ENDIAN__
> +static int openat(int dir_fd, const char *pathname, int flags, ...)
> +#else
> int openat(int dir_fd, const char *pathname, int flags, ...)
> +#endif
Well, that makes openat a private function - the idea however is to
provide a publicly visible function. Also, if I read the objdump output
correctly, the compiler will happily inline the function then as there
is just one usage.
> Hoping you'll get more ideas on this
After digging into this for quite a few hours, this really smells like a
bug in the optimizer but I'm not keen on spending a lot of time on a
reproducer and taking it to gcc upstream.
As a workaround I propose a tactical __attribute__ to locally disable
optimization as this issue has been blocking eatmydata for way too long,
and performance loss should be neglectable.
Status: Works on the porterbox.
Cheers,
Christoph
#ifdef HAVE_OPENAT
-int openat(int dir_fd, const char *pathname, int flags, ...)
+int
+#if HAVE_OPENAT && defined(__powerpc__) && defined(__powerpc64__) &&
__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+__attribute__((optimize("O0")))
+#endif
+openat(int dir_fd, const char *pathname, int flags, ...)
signature.asc
Description: PGP signature
Bug#995393: fakeroot: FTBFS on ppc64el
On Thu, Sep 30, 2021 at 04:46:47PM +0200, Frédéric Bonnard wrote: > Not sure what optimization breaks, I just tried changing openat() to > modify some aggressive optimization on that function and it helped. The optimization seems to break with -O1, but not with -O0 -fcombine-stack-adjustments -fcompare-elim -fcprop-registers -fdefer-pop -fforward-propagate -fguess-branch-probability -fipa-profile -fipa-pure-const -fipa-reference -fipa-reference-addressable -fmerge-constants -fomit-frame-pointer -freorder-blocks -fshrink-wrap -fsplit-wide-types -ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-coalesce-vars -ftree-copy-prop -ftree-dce -ftree-dominator-opts -ftree-fre -ftree-sink -ftree-slsr -ftree-ter -fbranch-count-reg -fdelayed-branch -fdse -fif-conversion -fif-conversion2 -finline-functions-called-once -fmove-loop-invariants -fssa-phiopt -fipa-modref -ftree-bit-ccp -ftree-dse -ftree-pta -ftree-sra It also seems fine at -O2 if fprintf(stderr, "%i, %s, %x\n", dir_fd, pathname, flags); is called prior to next_openat.
Bug#995393: fakeroot: FTBFS on ppc64el
Package: src:fakeroot
Version: 1.26-1
Control: tags -1 ftbfs
--
Dear maintainer,
fakeroot fails to build on ppc64el since 1.26-1
https://buildd.debian.org/status/fetch.php?pkg=fakeroot=ppc64el=1.26-1=1630982822=0
Backtrace from the core file :
---
Core was generated by `chown -R daemon:sys 2 '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x05e13c212284 in 001b.plt_call.fdopendir@@GLIBC_2.17 ()
(gdb) bt
#0 0x05e13c212284 in 001b.plt_call.fdopendir@@GLIBC_2.17 ()
#1 0x05e13c220fd4 in opendirat (dir_fd=-100, dir=0x5e13fb347b0 "2",
extra_flags=32768, pnew_fd=0x7fffc6e5cab0) at lib/opendirat.c:44
#2 0x05e13c21cb74 in fts_build (sp=0x5e13fb33ce0, type=3) at lib/fts.c:1340
#3 0x05e13c21be60 in rpl_fts_read (sp=0x5e13fb33ce0) at lib/fts.c:940
#4 0x05e13c214ca0 in chown_files (files=0x7fffc6e5d210, bit_flags=1040,
uid=1, gid=3, required_uid=4294967295, required_gid=4294967295,
chopt=0x7fffc6e5cd00) at src/chown-core.c:531
#5 0x05e13c21349c in main (argc=4, argv=0x7fffc6e5d1f8) at src/chown.c:324
---
Bisecting the code I ended up with this commit :
https://salsa.debian.org/clint/fakeroot/-/commit/f5e0a89ab6f0024f3d3bec5fd9cf631676b44f6c
from which things start to fail.
Especially the openat() function.
What I noticed also is that lowering optimization to O0 makes the test
work.
Simple way to reproduce is on directories :
---
mkdir bla
LD_PRELOAD=./obj-sysv/.libs/libfakeroot-0.so /bin/rm -fr bla
---
Each time fdopendir seg fault with the fd coming from openat right
before.
Not sure what optimization breaks, I just tried changing openat() to
modify some aggressive optimization on that function and it helped.
---
--- a/libfakeroot.c
+++ b/libfakeroot.c
@@ -2596,7 +2596,11 @@
#endif
#ifdef HAVE_OPENAT
+#if defined(__powerpc__) && defined(__powerpc64__) && __BYTE_ORDER__ ==
__ORDER_LITTLE_ENDIAN__
+static int openat(int dir_fd, const char *pathname, int flags, ...)
+#else
int openat(int dir_fd, const char *pathname, int flags, ...)
+#endif
{
mode_t mode;
---
Hoping you'll get more ideas on this
Regards,
F.
signature.asc
Description: PGP signature
