public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Joseph Myers <joseph@codesourcery.com>
To: <gcc-patches@gcc.gnu.org>
Subject: lto: Fix option merging [PR106129]
Date: Wed, 29 Jun 2022 20:44:10 +0000	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2206292042360.353138@digraph.polyomino.org.uk> (raw)

The LTO merging of options from different input files was broken by:

commit 227a2ecf663d69972b851f51f1934d18927b62cd
Author: Martin Liska <mliska@suse.cz>
Date:   Fri Mar 12 11:53:47 2021 +0100

    lto-wrapper: Use vec<cl_decoded_option> data type.

Previously, find_and_merge_options would merge options it read into
those in *opts. After this commit, options in *opts on entry to
find_and_merge_options are ignored; the only merging that takes place
is between multiple sets of options in the same input file that are
read in the same call to this function (not sure how that case can
occur at all). The effects include, for example, that if some objects
are built with PIC enabled and others with it disabled, and the last
LTO object processed has PIC enabled, the choice of PIC for the last
object will result in the whole program being built as PIC, when the
merging logic is intended to ensure that a mixture of PIC and non-PIC
objects results in the whole program being built as non-PIC.

Fix this with an extra argument to find_and_merge_options to determine
whether merging should take place.  This shows up a second issue with
that commit (which I think wasn't actually intended to change code
semantics at all): once merging is enabled again, the check for
-Xassembler options became an infinite loop in the case where both
inputs had -Xassembler options, with the same first option, so fix
that loop to restore the previous semantics.

Note that I'm not sure how LTO option merging might be tested in the
testsuite (clearly there wasn't sufficient, if any, coverage to detect
these bugs).

Bootstrapped with no regressions for x86_64-pc-linux-gnu.  OK to
commit?

	PR lto/106129
	* lto-wrapper.cc (find_option): Add argument start.
	(merge_and_complain): Loop over existing_opt_index and
	existing_opt2_index for Xassembler check.  Update calls to
	find_option.
	(find_and_merge_options): Add argument first to determine whether
	to merge options with those passed in *opts.
	(run_gcc): Update calls to find_and_merge_options.

diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
index 26e06e7..795ab74 100644
--- a/gcc/lto-wrapper.cc
+++ b/gcc/lto-wrapper.cc
@@ -170,13 +170,14 @@ get_options_from_collect_gcc_options (const char *collect_gcc,
   return decoded;
 }
 
-/* Find option in OPTIONS based on OPT_INDEX.  -1 value is returned
-   if the option is not present.  */
+/* Find option in OPTIONS based on OPT_INDEX, starting at START.  -1
+   value is returned if the option is not present.  */
 
 static int
-find_option (vec<cl_decoded_option> &options, size_t opt_index)
+find_option (vec<cl_decoded_option> &options, size_t opt_index,
+	     unsigned start = 0)
 {
-  for (unsigned i = 0; i < options.length (); ++i)
+  for (unsigned i = start; i < options.length (); ++i)
     if (options[i].opt_index == opt_index)
       return i;
 
@@ -575,13 +576,16 @@ merge_and_complain (vec<cl_decoded_option> &decoded_options,
    else
      j++;
 
+  int existing_opt_index, existing_opt2_index;
   if (!xassembler_options_error)
-    for (i = j = 0; ; i++, j++)
+    for (existing_opt_index = existing_opt2_index = 0; ;
+	 existing_opt_index++, existing_opt2_index++)
       {
-	int existing_opt_index
-	  = find_option (decoded_options, OPT_Xassembler);
-	int existing_opt2_index
-	  = find_option (fdecoded_options, OPT_Xassembler);
+	existing_opt_index
+	  = find_option (decoded_options, OPT_Xassembler, existing_opt_index);
+	existing_opt2_index
+	  = find_option (fdecoded_options, OPT_Xassembler,
+			 existing_opt2_index);
 
 	cl_decoded_option *existing_opt = NULL;
 	cl_decoded_option *existing_opt2 = NULL;
@@ -1100,7 +1104,7 @@ find_crtoffloadtable (int save_temps, const char *dumppfx)
 
 static bool
 find_and_merge_options (int fd, off_t file_offset, const char *prefix,
-			vec<cl_decoded_option> decoded_cl_options,
+			vec<cl_decoded_option> decoded_cl_options, bool first,
 			vec<cl_decoded_option> *opts, const char *collect_gcc)
 {
   off_t offset, length;
@@ -1110,6 +1114,9 @@ find_and_merge_options (int fd, off_t file_offset, const char *prefix,
   int err;
   vec<cl_decoded_option> fdecoded_options;
 
+  if (!first)
+    fdecoded_options = *opts;
+
   simple_object_read *sobj;
   sobj = simple_object_start_read (fd, file_offset, "__GNU_LTO",
 				   &errmsg, &err);
@@ -1130,7 +1137,6 @@ find_and_merge_options (int fd, off_t file_offset, const char *prefix,
   data = (char *)xmalloc (length);
   read (fd, data, length);
   fopts = data;
-  bool first = true;
   do
     {
       vec<cl_decoded_option> f2decoded_options
@@ -1417,8 +1423,10 @@ run_gcc (unsigned argc, char *argv[])
   int auto_parallel = 0;
   bool no_partition = false;
   const char *jobserver_error = NULL;
+  bool fdecoded_options_first = true;
   vec<cl_decoded_option> fdecoded_options;
   fdecoded_options.create (16);
+  bool offload_fdecoded_options_first = true;
   vec<cl_decoded_option> offload_fdecoded_options = vNULL;
   struct obstack argv_obstack;
   int new_head_argc;
@@ -1510,11 +1518,13 @@ run_gcc (unsigned argc, char *argv[])
 	}
 
       if (find_and_merge_options (fd, file_offset, LTO_SECTION_NAME_PREFIX,
-				  decoded_options, &fdecoded_options,
+				  decoded_options, fdecoded_options_first,
+				  &fdecoded_options,
 				  collect_gcc))
 	{
 	  have_lto = true;
 	  ltoobj_argv[ltoobj_argc++] = argv[i];
+	  fdecoded_options_first = false;
 	}
       close (fd);
     }
@@ -1773,9 +1783,12 @@ cont1:
 	    fatal_error (input_location, "cannot open %s: %m", filename);
 	  if (!find_and_merge_options (fd, file_offset,
 				       OFFLOAD_SECTION_NAME_PREFIX,
-				       decoded_options, &offload_fdecoded_options,
+				       decoded_options,
+				       offload_fdecoded_options_first,
+				       &offload_fdecoded_options,
 				       collect_gcc))
 	    fatal_error (input_location, "cannot read %s: %m", filename);
+	  offload_fdecoded_options_first = false;
 	  close (fd);
 	  if (filename != offload_argv[i])
 	    XDELETEVEC (filename);

-- 
Joseph S. Myers
joseph@codesourcery.com

             reply	other threads:[~2022-06-29 20:44 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-29 20:44 Joseph Myers [this message]
2022-06-30  5:31 ` Richard Biener

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.22.394.2206292042360.353138@digraph.polyomino.org.uk \
    --to=joseph@codesourcery.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).