Fix problem with COFF and plugins

Message ID 0390fe12-85dc-6e03-5d02-7f3d9bad85fc@codesourcery.com
State New
Headers show
Series
  • Fix problem with COFF and plugins
Related show

Commit Message

Andrew Jenner June 10, 2019, 4:41 p.m.
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?

Thanks,

Andrew


ld/ChangeLog

2018-06-10  Andrew Jenner  <andrew@codesourcery.com>

         * ldfile.c (ldfile_try_open_bfd): Reset plugin_format to
         bfd_plugin_unknown before calling plugin_maybe_claim.

Comments

Alan Modra June 11, 2019, 10:41 a.m. | #1
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
Alan Modra June 28, 2019, 2:12 a.m. | #2
On Tue, Jun 11, 2019 at 08:11:31PM +0930, Alan Modra wrote:
> 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?


I didn't get a reply to this, so I'm going ahead with committing the
patch.

> 	* 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.


-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/ld/ldfile.c b/ld/ldfile.c
index a72ff13..eab0740 100644
--- a/ld/ldfile.c
+++ b/ld/ldfile.c
@@ -315,7 +315,13 @@  success:
   if (link_info.lto_plugin_active
       && !no_more_claiming
       && bfd_check_format (entry->the_bfd, bfd_object))
-    plugin_maybe_claim (entry);
+    {
+      /* The target of the plugin_dummy_bfd may have been
+	 determined based on incomplete information.  Force
+	 it to be recomputed now that the target is known.  */
+      entry->the_bfd->plugin_format = bfd_plugin_unknown;
+      plugin_maybe_claim (entry);
+    }
 #endif /* ENABLE_PLUGINS */
 
   /* It opened OK, the format checked out, and the plugins have had