PATCH: fix race condition in binutils-2.17 build

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

PATCH: fix race condition in binutils-2.17 build

Marc Espie-2
We don't normally hit this thanks to the "expensive job" heuristics
(meaning that we don't start the second target while the first is
building), but still there is a race, and if for whatever reason we
remove that heuristics we hit it fairly hard.

The patch is fairly trivial, just run both targets separately.
Example of a crash and analysis follows.

Note that the problem only occurs in binutils-2.17. The former versions
did have one less level of indirection in their Makefiles.

Index: Makefile.bsd-wrapper
===================================================================
RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/Makefile.bsd-wrapper,v
retrieving revision 1.19
diff -u -p -r1.19 Makefile.bsd-wrapper
--- Makefile.bsd-wrapper 21 Dec 2019 21:40:00 -0000 1.19
+++ Makefile.bsd-wrapper 2 Jan 2020 10:13:01 -0000
@@ -64,6 +64,7 @@ CONFIGTARGET+=--without-gnu-as
 .endif
 
 all: config.status
+.for t in all info
  SUBDIRS='${SUBDIRS}' \
   CONFIGURE_HOST_MODULES='${CONFIGURE_HOST_MODULES}' \
   ${MAKE} ${XCFLAGS} \
@@ -74,7 +75,8 @@ all: config.status
     BSDSRCDIR=${BSDSRCDIR} \
     ALL_MODULES="${ALL_MODULES}" \
     ALL_HOST_MODULES='${ALL_HOST_MODULES}' \
-    INFO_HOST_MODULES='${INFO_HOST_MODULES}' all info
+    INFO_HOST_MODULES='${INFO_HOST_MODULES}' $t
+.endfor
 
 .include <bsd.own.mk>
 
@@ -115,6 +117,7 @@ config.status: do-config
 
 # Need to pass SUBDIRS because of install-info
 install: maninstall
+.for t in install install-info
  ${INSTALL} -d -o ${BINOWN} -g ${BINGRP} \
     ${DESTDIR}${PREFIX}/libdata/ldscripts
  SUBDIRS='${INST_SUBDIRS}' ${MAKE} DESTDIR='${DESTDIR}' \
@@ -126,7 +129,8 @@ install: maninstall
     INSTALL_PROGRAM='${INSTALL} -c ${INSTALL_STRIP} -o ${BINOWN} -g ${BINGRP} -m ${BINMODE}' \
     INSTALL_DATA='${INSTALL} -c -o ${DOCOWN} -g ${DOCGRP} -m ${NONBINMODE}' \
     INSTALL_INFO_HOST_MODULES='${INSTALL_INFO_HOST_MODULES}' \
-      install install-info
+      $t
+.endfor
 .if ${LINKER_VERSION:L} == "bfd"
  rm -f ${DESTDIR}${PREFIX}/bin/ld
  ln ${DESTDIR}${PREFIX}/bin/ld.bfd ${DESTDIR}${PREFIX}/bin/ld


Tail of a crashing build:
checking for LC_MESSAGES... (cached) yes
checking for LC_MESSAGES... (cached) yes
checking whether NLS is requested... no
checking whether NLS is requested... no
updating cache ./config.cache
updating cache ./config.cache
creating ./config.status
creating ./config.status
mv: rename conftest.tail to conftest.vals: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
mv: conftest.tail: No such file or directory
creating Makefile
creating Makefile
./config.status[812]: syntax error: `fi' unexpected
./config.status[812]: syntax error: `fi' unexpected
*** Error 1 in gnu/usr.bin/binutils-2.17/obj (Makefile:5406 'configure-intl': @r=`${PWDCMD-pwd}`; export r;  s=`cd /usr/src/gnu/usr.bin/binu...)
*** Error 1 in gnu/usr.bin/binutils-2.17/obj (Makefile:5406 'configure-intl': @r=`${PWDCMD-pwd}`; export r;  s=`cd /usr/src/gnu/usr.bin/binu...)
*** Error 2 in target 'all'
*** Error 2 in gnu/usr.bin/binutils-2.17/obj (Makefile:747 'do-info': @r=`${PWDCMD-pwd}`; export r;  s=`cd /usr/src/gnu/usr.bin/binutils-2.1...)
*** Error 2 (Makefile:634 'all': @r=`${PWDCMD-pwd}`; export r;  s=`cd /usr/src/gnu/usr.bin/binutils-2.17; ${PWDCMD-pwd}`; export s;  if [ -f...)
*** Error 2 in gnu/usr.bin/binutils-2.17 (Makefile.bsd-wrapper:77 'all')

You'll notice the duplicate lines. It's actually libintl running configure
twice at the same time.  Initially, I thought "oh let's configure libintl
up-front". Unfortunately this is not enough, and leads to a different crash.


Now, for the generated Makefile. In binutils prior to 2.17, both all and
info go straight info building, so they're not affected.

binutils-2.17 has the following snippets:

all:
        @: $(MAKE); $(unstage)
        @r=`${PWD_COMMAND}`; export r; \
        s=`cd $(srcdir); ${PWD_COMMAND}`; export s; \
        if [ -f stage_last ]; then \
          $(MAKE) $(TARGET_FLAGS_TO_PASS) all-host all-target; \
        else \
          $(MAKE) $(RECURSE_FLAGS_TO_PASS) all-host all-target; \
        fi

(the : $(MAKE) is the fun way automake/autoconf disables maintenance lines)

and

info: do-info
do-info:
        @: $(MAKE); $(unstage)
        @r=`${PWD_COMMAND}`; export r; \
        s=`cd $(srcdir); ${PWD_COMMAND}`; export s; \
        $(MAKE) $(RECURSE_FLAGS_TO_PASS) info-host \
          info-target

so, if they're run at the same time, they will *both* recurse and run
a separate make...

looking a bit further, info-host and all-host do share a lot of dependencies,
so BOOM.

Reply | Threaded
Open this post in threaded view
|

Re: PATCH: fix race condition in binutils-2.17 build

Philip Guenther-2
On Thu, Jan 2, 2020 at 2:26 AM Marc Espie <[hidden email]> wrote:

> We don't normally hit this thanks to the "expensive job" heuristics
> (meaning that we don't start the second target while the first is
> building), but still there is a race, and if for whatever reason we
> remove that heuristics we hit it fairly hard.
>
> The patch is fairly trivial, just run both targets separately.
> Example of a crash and analysis follows.
>
> Note that the problem only occurs in binutils-2.17. The former versions
> did have one less level of indirection in their Makefiles.
>

ok guenther@