Re: [PATCH] Fix problem with COFF and plugins

[Alan Modra <amodra at gmail dot com>]

On Mon, Jun 10, 2019 at 05:41:06PM +0100, Andrew Jenner wrote:
> Since the change
> https://www.sourceware.org/ml/binutils/2018-05/msg00166.html, LTO has been
> broken for mingw32 targets specifically (and probably other COFF targets as
> well). The error is "file not recognized: file format not recognized" for
> the object file that will be claimed by the plugin. I investigated, and what
> is going on is as follows:
> 
> The object file is recognized as target pe-i386 by the loop in
> bfd_check_format_matches(). However (since this change) we call
> _bfd_check_format with the plugin target as well. This causes the input
> BFD's plugin_format to be set to bfd_plugin_yes. However, because the target
> of the input file is not yet known, the target of the input BFD's
> plugin_dummy_bfd is set (via the fallback case in plugin_get_ir_dummy_bfd())
> to link_info.output_bfd which in this case is pei-i386 (the output format)
> instead of pe-i386 (this is not a problem for ELF because the output and
> object formats are the same).
> 
> Then, when plugin_maybe_claim() is called, the real file's BFD is replaced
> with the dummy one (with the wrong format) and the file doesn't match the
> format.
> 
> There are a few different ways that this could be fixed, and I'm not sure
> which one is preferred. One would be to change bfd_check_format_matches() to
> not check the plugin format if we've already found a better format. I don't
> think that would affect the fix to PR22458.
> 
> Another possible fix (for which a patch is attached) is to reset
> plugin_format after the call to bfd_check_format, so that it is recomputed
> (this time with sufficient information) and the plugin_dummy_bfd's target is
> correct. We have done a test pass with this patch and it didn't cause any
> problems. Is this change okay to commit, or would a change to
> bfd_check_format_matches() be preferred?

I think I prefer to change bfd_check_format_matches.  Since I don't
have a good test environment for mingw, would you please test whether
the following is good?

	* format.c (bfd_check_format_matches): Don't match plugin target
	if another target matches.  Expand comment.
	* targets.c (_bfd_target_vector): Move plugin_vec after all other
	non-corefile targets, outside !SELECT_VECS.
	* config.bfd: Don't handle targ=plugin here.
	* configure.ac: Don't add plugin to enable_targets or handle in
	target loop setting selvecs and other target vars.
	* configure: Regenerate.

diff --git a/bfd/config.bfd b/bfd/config.bfd
index c6b04ea4a5..13d678e1f8 100644
--- a/bfd/config.bfd
+++ b/bfd/config.bfd
@@ -218,11 +218,6 @@ esac
 #  convention, else the table becomes a real mess to understand and maintain.
 
 case "${targ}" in
-  plugin)
-    targ_defvec=plugin_vec
-    targ_selvecs="plugin_vec"
-    ;;
-
 # START OF targmatch.h
 #ifdef BFD64
   aarch64-*-darwin*)
diff --git a/bfd/configure b/bfd/configure
index 6f95045e59..b1a727a54a 100755
--- a/bfd/configure
+++ b/bfd/configure
@@ -12409,10 +12409,6 @@ else
 fi
 
 
-if test "$plugins" = "yes"; then
-  enable_targets="$enable_targets plugin"
-fi
-
 # Check whether --enable-64-bit-bfd was given.
 if test "${enable_64_bit_bfd+set}" = set; then :
   enableval=$enable_64_bit_bfd; case "${enableval}" in
@@ -14613,12 +14609,12 @@ selarchs=
 TDEFINES=
 for targ in $target $canon_targets
 do
-    if test "x$targ" = "xall"; then
+    if test $targ = all; then
         all_targets=true
 	assocvecs="$assocvecs $targ_defvec $targ_selvecs"
-    else
+    elif test $targ != plugin; then
 	. $srcdir/config.bfd
-	if test "x$targ" = "x$target"; then
+	if test $targ = $target; then
 	    defvec=$targ_defvec
 	fi
 	selvecs="$selvecs $targ_defvec $targ_selvecs"
@@ -14856,7 +14852,6 @@ do
     pef_xlib_vec)		 tb="$tb pef.lo" ;;
     pj_elf32_vec)		 tb="$tb elf32-pj.lo elf32.lo $elf" ;;
     pj_elf32_le_vec)		 tb="$tb elf32-pj.lo elf32.lo $elf" ;;
-    plugin_vec)			 tb="$tb plugin.lo" ;;
     powerpc_boot_vec)		 tb="$tb ppcboot.lo" ;;
     powerpc_elf32_vec)		 tb="$tb elf32-ppc.lo elf-vxworks.lo elf32.lo $elf" ;;
     powerpc_elf32_le_vec)	 tb="$tb elf32-ppc.lo elf-vxworks.lo elf32.lo $elf" ;;
@@ -14983,6 +14978,10 @@ do
     fi
 done
 
+if test "$plugins" = "yes"; then
+     tb="$tb plugin.lo"
+fi
+
 # Target architecture .o files.
 # A couple of CPUs use shorter file names to avoid problems on DOS
 # filesystems.
diff --git a/bfd/configure.ac b/bfd/configure.ac
index c941389138..39702ce131 100644
--- a/bfd/configure.ac
+++ b/bfd/configure.ac
@@ -46,10 +46,6 @@ ACX_LARGEFILE
 
 AM_CONDITIONAL(PLUGINS, test "$plugins" = "yes")
 
-if test "$plugins" = "yes"; then
-  enable_targets="$enable_targets plugin"
-fi
-
 AC_ARG_ENABLE(64-bit-bfd,
 [  --enable-64-bit-bfd     64-bit support (on hosts with narrower word sizes)],
 [case "${enableval}" in
@@ -349,12 +345,12 @@ selarchs=
 TDEFINES=
 for targ in $target $canon_targets
 do
-    if test "x$targ" = "xall"; then
+    if test $targ = all; then
         all_targets=true
 	assocvecs="$assocvecs $targ_defvec $targ_selvecs"
-    else
+    elif test $targ != plugin; then
 	. $srcdir/config.bfd
-	if test "x$targ" = "x$target"; then
+	if test $targ = $target; then
 	    defvec=$targ_defvec
 	fi
 	selvecs="$selvecs $targ_defvec $targ_selvecs"
@@ -592,7 +588,6 @@ do
     pef_xlib_vec)		 tb="$tb pef.lo" ;;
     pj_elf32_vec)		 tb="$tb elf32-pj.lo elf32.lo $elf" ;;
     pj_elf32_le_vec)		 tb="$tb elf32-pj.lo elf32.lo $elf" ;;
-    plugin_vec)			 tb="$tb plugin.lo" ;;
     powerpc_boot_vec)		 tb="$tb ppcboot.lo" ;;
     powerpc_elf32_vec)		 tb="$tb elf32-ppc.lo elf-vxworks.lo elf32.lo $elf" ;;
     powerpc_elf32_le_vec)	 tb="$tb elf32-ppc.lo elf-vxworks.lo elf32.lo $elf" ;;
@@ -719,6 +714,10 @@ do
     fi
 done
 
+if test "$plugins" = "yes"; then
+     tb="$tb plugin.lo"
+fi
+
 # Target architecture .o files.
 # A couple of CPUs use shorter file names to avoid problems on DOS
 # filesystems.
diff --git a/bfd/format.c b/bfd/format.c
index 97a92291a8..1d1363d184 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -290,8 +290,15 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
     {
       const bfd_target *temp;
 
-      /* Don't check the default target twice.  */
+      /* The binary target matches anything, so don't return it when
+	 searching.  Don't match the plugin target if we have another
+	 alternative since we want to properly set the input format
+	 before allowing a plugin to claim the file.  Also, don't
+	 check the default target twice.  */
       if (*target == &binary_vec
+#if BFD_SUPPORTS_PLUGINS
+	  || (match_count != 0 && *target == &plugin_vec)
+#endif
 	  || (!abfd->target_defaulted && *target == save_targ))
 	continue;
 
diff --git a/bfd/targets.c b/bfd/targets.c
index d3d52a5e2a..6b85c62798 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -1149,10 +1149,6 @@ static const bfd_target * const _bfd_target_vector[] =
 	&pj_elf32_vec,
 	&pj_elf32_le_vec,
 
-#if BFD_SUPPORTS_PLUGINS
-	&plugin_vec,
-#endif
-
 	&powerpc_boot_vec,
 	&powerpc_elf32_vec,
 	&powerpc_elf32_le_vec,
@@ -1305,6 +1301,10 @@ static const bfd_target * const _bfd_target_vector[] =
 /* Likewise for ihex.  */
 	&ihex_vec,
 
+#if BFD_SUPPORTS_PLUGINS
+	&plugin_vec,
+#endif
+
 /* Add any required traditional-core-file-handler.  */
 
 #ifdef AIX386_CORE

-- 
Alan Modra
Australia Development Lab, IBM