Bug#893557: thin-provisioning-tools: FTBFS with glibc 2.17

2019-01-13 Thread Bastian Blank 
Hi Steve

On Tue, Mar 20, 2018 at 02:10:52PM -0700, Steve Langasek wrote:
> Considering debian/rules also hardcodes -O2 for the package itself, it
> doesn't seem inappropriate to also build the test shim with the same
> options?
> Yes, this is working around a toolchain issue.  Feel free to reassign this
> bug to the toolchain, if you prefer.

I digged deeper and the arguments do not matter.  The whole problem is
not a toolchain issue, it's actually triggered by a small different in
the used prototypes.

> FWIW I was able to identify a symbol reference that drops out of the .so
> when built with -O3 but is present when built with -O2; this may be the
> cause of the crash:
> -  DF *UND*   GLIBC_2.17  0x60 
> __stack_chk_fail
> And perhaps that points to an explanation of the toolchain bug.

In my own tests this different does not show up.  Also it's a undefined
symbol, so it is for a call into something else.

> It's annoyingly difficult to debug this crash, as gdb doesn't appear to let
> me DTRT with 'set env LD_PRELOAD' and instead manages to crash itself before
> handing control over to the process under test.

Yes, it is.  And adding debugging output makes it disappear.

Anyway, the cause is:

| static int (*orig_open)(const char *pathname, int flags, int mode);

This does not match the public prototype for the open function.

Regards,
Bastian

-- 
Insufficient facts always invite danger.
-- Spock, "Space Seed", stardate 3141.9

Bug#893557: thin-provisioning-tools: FTBFS with glibc 2.17

2018-03-20 Thread Steve Langasek 
On Tue, Mar 20, 2018 at 08:47:41AM +0100, Bastian Blank wrote:
> Hi Steve

> On Mon, Mar 19, 2018 at 03:37:17PM -0700, Steve Langasek wrote:
> > Ok, after some trial and error, I've determined that the reason my earlier
> > test succeeded was because I had built the test preload.so without the
> > benefit of dpkg-buildflags, which means -O3, which is used by default for
> > ppc64el on Ubuntu, was missing from the commandline.

> It is a bug in the compiler if it fails to compile a valid program with
> -O3.  So I don't see what this would fix.

Considering debian/rules also hardcodes -O2 for the package itself, it
doesn't seem inappropriate to also build the test shim with the same
options?

Yes, this is working around a toolchain issue.  Feel free to reassign this
bug to the toolchain, if you prefer.

FWIW I was able to identify a symbol reference that drops out of the .so
when built with -O3 but is present when built with -O2; this may be the
cause of the crash:

-  DF *UND*   GLIBC_2.17  0x60 
__stack_chk_fail

And perhaps that points to an explanation of the toolchain bug.

It's annoyingly difficult to debug this crash, as gdb doesn't appear to let
me DTRT with 'set env LD_PRELOAD' and instead manages to crash itself before
handing control over to the process under test.

-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developerhttp://www.debian.org/
slanga...@ubuntu.com vor...@debian.org


signature.asc
Description: PGP signature

Bug#893557: thin-provisioning-tools: FTBFS with glibc 2.17

2018-03-20 Thread Bastian Blank 
Hi Steve

On Mon, Mar 19, 2018 at 03:37:17PM -0700, Steve Langasek wrote:
> Ok, after some trial and error, I've determined that the reason my earlier
> test succeeded was because I had built the test preload.so without the
> benefit of dpkg-buildflags, which means -O3, which is used by default for
> ppc64el on Ubuntu, was missing from the commandline.

It is a bug in the compiler if it fails to compile a valid program with
-O3.  So I don't see what this would fix.

Bastian

-- 
Most legends have their basis in facts.
-- Kirk, "And The Children Shall Lead", stardate 5029.5

Bug#893557: thin-provisioning-tools: FTBFS with glibc 2.17

2018-03-19 Thread Steve Langasek 
Package: thin-provisioning-tools
Version: 0.7.4-2
Followup-For: Bug #893557
User: ubuntu-de...@lists.ubuntu.com
Usertags: origin-ubuntu bionic ubuntu-patch

Control: retitle -1 thin-provisioning-tools: FTBFS with -O3
Control: severity -1 minor
Control: tags -1 + patch

Ok, after some trial and error, I've determined that the reason my earlier
test succeeded was because I had built the test preload.so without the
benefit of dpkg-buildflags, which means -O3, which is used by default for
ppc64el on Ubuntu, was missing from the commandline.

The attached patch supersedes the previous one, and fixes the build failure
on Ubuntu ppc64el.  Please consider including it in Debian as well for
benefit of anyone who happens to be using -O3 in dpkg-buildflags (not
actually relevant in the distro case in Debian).

-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developerhttp://www.debian.org/
slanga...@ubuntu.com vor...@debian.org
diff -Nru thin-provisioning-tools-0.7.4/debian/control 
thin-provisioning-tools-0.7.4/debian/control
--- thin-provisioning-tools-0.7.4/debian/control2017-11-01 
14:50:39.0 -0700
+++ thin-provisioning-tools-0.7.4/debian/control2018-03-19 
15:31:50.0 -0700
@@ -1,8 +1,7 @@
 Source: thin-provisioning-tools
 Section: admin
 Priority: optional
-Maintainer: Ubuntu Developers 
-XSBC-Original-Maintainer: Debian LVM Team 

+Maintainer: Debian LVM Team 
 Uploaders: Bastian Blank 
 Build-Depends:
  debhelper (>= 9),
diff -Nru thin-provisioning-tools-0.7.4/debian/rules 
thin-provisioning-tools-0.7.4/debian/rules
--- thin-provisioning-tools-0.7.4/debian/rules  2017-11-01 14:50:39.0 
-0700
+++ thin-provisioning-tools-0.7.4/debian/rules  2018-03-19 15:30:07.0 
-0700
@@ -15,7 +15,9 @@
--with-optimisation=-O2
 
 override_dh_auto_test:
-   $(MAKE) -C debian/unit-tests all
+   $(MAKE) -C debian/unit-tests all \
+   CFLAGS="$$(env DEB_CFLAGS_MAINT_STRIP=-O3 \
+   dpkg-buildflags --get CFLAGS)"
+LD_PRELOAD=$(CURDIR)/debian/unit-tests/preload.so $(MAKE) unit-test 
GMOCK_DIR=/usr/src/googletest
 
 override_dh_auto_clean:

Bug#893557: thin-provisioning-tools: FTBFS with glibc 2.17

2018-03-19 Thread Steve Langasek 
Control: tags -1 - patch
Control: severity -1 serious

On Mon, Mar 19, 2018 at 02:19:40PM -0700, Steve Langasek wrote:
> Package: thin-provisioning-tools
> Version: 0.7.4-2
> Severity: important
> Tags: patch
> User: ubuntu-de...@lists.ubuntu.com
> Usertags: origin-ubuntu bionic ubuntu-patch

> Hi Bastian,

> In Ubuntu, thin-provisioning-tools 0.7.4-2 fails to build on ppc64el with an
> ICE in the compiler while building the test suite.  An strace implies that
> this is because your LD_PRELOAD wrapper has interposed open(), but not
> openat().

> If I extend the wrapper as in the attached patch to cover openat(), the
> package now builds.

> This build failure is probably not reproducible yet in Debian, but I expect
> it will be once Debian moves to glibc 2.17.

Hmm, sorry, several corrections:

- the correct glibc version number in 2.27, not 2.17.
- Debian also has 2.27 now, so the build failure probably is reproducible in
  Debian.
- I managed to accidentally create and test this patch against
  thin-provisioning-tools 0.6.1-4 only, the last version that successfully
  built in Ubuntu.  The patch solves the build failure there... but does not
  solve the build failure in 0.7.4, for reasons that currently elude me.

> I briefly looked at whether your custom LD_PRELOAD wrapper could be replaced
> with libeatmydata, but I see that libeatmydata only handles O_SYNC and
> O_DSYNC, not O_DIRECT.  Do you think it would make sense to ask libeatmydata
> to filter O_DIRECT as well, to reduce the need for maintaining this separate
> .so?
> 
> -- 
> Steve Langasek   Give me a lever long enough and a Free OS
> Debian Developer   to set it on, and I can move the world.
> Ubuntu Developerhttp://www.debian.org/
> slanga...@ubuntu.com vor...@debian.org

> diff -Nru thin-provisioning-tools-0.7.4/debian/control 
> thin-provisioning-tools-0.7.4/debian/control
> --- thin-provisioning-tools-0.7.4/debian/control  2017-11-01 
> 14:50:39.0 -0700
> +++ thin-provisioning-tools-0.7.4/debian/control  2018-03-19 
> 14:10:28.0 -0700
> @@ -1,8 +1,7 @@
>  Source: thin-provisioning-tools
>  Section: admin
>  Priority: optional
> -Maintainer: Ubuntu Developers 
> -XSBC-Original-Maintainer: Debian LVM Team 
> 
> +Maintainer: Debian LVM Team 
>  Uploaders: Bastian Blank 
>  Build-Depends:
>   debhelper (>= 9),
> diff -Nru thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c 
> thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c
> --- thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c 2017-11-01 
> 14:50:39.0 -0700
> +++ thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c 2018-03-19 
> 14:10:17.0 -0700
> @@ -4,13 +4,17 @@
>  #include 
>  #include 
>  
> -static int (*orig_open)(const char *pathname, int flags, int mode);
> +static int (*orig_open)(const char *pathname, int flags, mode_t mode);
> +static int (*orig_openat)(int dirfd, const char *pathname, int flags, mode_t 
> mode);
>  
>  __attribute__((constructor))
>  static void constructor() {
>orig_open = dlsym(RTLD_NEXT, "open");
>if (!orig_open)
>  abort();
> +  orig_openat = dlsym(RTLD_NEXT, "openat");
> +  if (!orig_openat)
> +abort();
>  }
>  
>  int open(const char *file, int oflag, ...) {
> @@ -51,3 +55,34 @@
>  int __open64_2(const char *file, int oflag) {
>return orig_open(file, (oflag & ~O_DIRECT) | O_LARGEFILE, 0);
>  }
> +
> +int openat(int dirfd, const char *file, int oflag, ...) {
> +  int mode = 0;
> +
> +  oflag &= ~O_DIRECT;
> +
> +  if (oflag & O_CREAT) {
> +va_list arg;
> +va_start(arg, oflag);
> +mode = va_arg(arg, int);
> +va_end(arg);
> +  }
> +
> +  return orig_openat(dirfd, file, oflag, mode);
> +}
> +
> +int openat64(int dirfd, const char *file, int oflag, ...) {
> +  int mode = 0;
> +
> +  oflag |= O_LARGEFILE;
> +  oflag &= ~O_DIRECT;
> +
> +  if (oflag & O_CREAT) {
> +va_list arg;
> +va_start(arg, oflag);
> +mode = va_arg(arg, int);
> +va_end(arg);
> +  }
> +
> +  return orig_openat(dirfd, file, oflag, mode);
> +}


-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developerhttp://www.debian.org/
slanga...@ubuntu.com vor...@debian.org


signature.asc
Description: PGP signature

Bug#893557: thin-provisioning-tools: FTBFS with glibc 2.17

2018-03-19 Thread Steve Langasek 
Package: thin-provisioning-tools
Version: 0.7.4-2
Severity: important
Tags: patch
User: ubuntu-de...@lists.ubuntu.com
Usertags: origin-ubuntu bionic ubuntu-patch

Hi Bastian,

In Ubuntu, thin-provisioning-tools 0.7.4-2 fails to build on ppc64el with an
ICE in the compiler while building the test suite.  An strace implies that
this is because your LD_PRELOAD wrapper has interposed open(), but not
openat().

If I extend the wrapper as in the attached patch to cover openat(), the
package now builds.

This build failure is probably not reproducible yet in Debian, but I expect
it will be once Debian moves to glibc 2.17.

I briefly looked at whether your custom LD_PRELOAD wrapper could be replaced
with libeatmydata, but I see that libeatmydata only handles O_SYNC and
O_DSYNC, not O_DIRECT.  Do you think it would make sense to ask libeatmydata
to filter O_DIRECT as well, to reduce the need for maintaining this separate
.so?

-- 
Steve Langasek   Give me a lever long enough and a Free OS
Debian Developer   to set it on, and I can move the world.
Ubuntu Developerhttp://www.debian.org/
slanga...@ubuntu.com vor...@debian.org
diff -Nru thin-provisioning-tools-0.7.4/debian/control 
thin-provisioning-tools-0.7.4/debian/control
--- thin-provisioning-tools-0.7.4/debian/control2017-11-01 
14:50:39.0 -0700
+++ thin-provisioning-tools-0.7.4/debian/control2018-03-19 
14:10:28.0 -0700
@@ -1,8 +1,7 @@
 Source: thin-provisioning-tools
 Section: admin
 Priority: optional
-Maintainer: Ubuntu Developers 
-XSBC-Original-Maintainer: Debian LVM Team 

+Maintainer: Debian LVM Team 
 Uploaders: Bastian Blank 
 Build-Depends:
  debhelper (>= 9),
diff -Nru thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c 
thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c
--- thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c   2017-11-01 
14:50:39.0 -0700
+++ thin-provisioning-tools-0.7.4/debian/unit-tests/preload.c   2018-03-19 
14:10:17.0 -0700
@@ -4,13 +4,17 @@
 #include 
 #include 
 
-static int (*orig_open)(const char *pathname, int flags, int mode);
+static int (*orig_open)(const char *pathname, int flags, mode_t mode);
+static int (*orig_openat)(int dirfd, const char *pathname, int flags, mode_t 
mode);
 
 __attribute__((constructor))
 static void constructor() {
   orig_open = dlsym(RTLD_NEXT, "open");
   if (!orig_open)
 abort();
+  orig_openat = dlsym(RTLD_NEXT, "openat");
+  if (!orig_openat)
+abort();
 }
 
 int open(const char *file, int oflag, ...) {
@@ -51,3 +55,34 @@
 int __open64_2(const char *file, int oflag) {
   return orig_open(file, (oflag & ~O_DIRECT) | O_LARGEFILE, 0);
 }
+
+int openat(int dirfd, const char *file, int oflag, ...) {
+  int mode = 0;
+
+  oflag &= ~O_DIRECT;
+
+  if (oflag & O_CREAT) {
+va_list arg;
+va_start(arg, oflag);
+mode = va_arg(arg, int);
+va_end(arg);
+  }
+
+  return orig_openat(dirfd, file, oflag, mode);
+}
+
+int openat64(int dirfd, const char *file, int oflag, ...) {
+  int mode = 0;
+
+  oflag |= O_LARGEFILE;
+  oflag &= ~O_DIRECT;
+
+  if (oflag & O_CREAT) {
+va_list arg;
+va_start(arg, oflag);
+mode = va_arg(arg, int);
+va_end(arg);
+  }
+
+  return orig_openat(dirfd, file, oflag, mode);
+}