[PATCH v1] gold: Make '--section-ordering-file' also specifiable in env variable

[PATCH v1] gold: Make '--section-ordering-file' also specifiable in env variable

[Noah Goldstein]

New proposed env variable is 'GLOBAL_SECTION_ORDERING_FILE'.

The motivation for this change to help package maintainers use the
section ordering feature. For package maintainers, modifying the link
flags of all their maintained projects, is time consumining to a
degree that it is often considered infeasable. By making the feature
enablable simply with an environment variable, removes this
roadblock. Similiarly, the rationale behind ignoring errors when using
the environment specified file is to make it so that it never create
new problems in the build process.

The hope by making this feature more accessable it will allow package
maintainers to use ordering lists from profiles of their applications
to distribute better optimized packages.
---
 gold/gold.cc       |  2 +-
 gold/layout.cc     | 17 +++++++++++-----
 gold/layout.h      |  2 +-
 gold/main.cc       |  7 +++++--
 gold/options.h     |  4 +++-
 gold/output.cc     |  2 +-
 gold/parameters.cc | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 gold/parameters.h  | 39 ++++++++++++++++++++++++++++++++++-
 8 files changed, 112 insertions(+), 12 deletions(-)

diff --git a/gold/gold.cc b/gold/gold.cc
index 9fcb1f272d6..c1a6f448d28 100644
--- a/gold/gold.cc
+++ b/gold/gold.cc
@@ -568,7 +568,7 @@ queue_middle_tasks(const General_options& options,
      also specified, do not do anything here.  */
   if (parameters->options().has_plugins()
       && layout->is_section_ordering_specified()
-      && !parameters->options().section_ordering_file ())
+      && !parameters->get_section_ordering_file ())
     {
       for (Layout::Section_list::const_iterator p
 	     = layout->section_list().begin();
diff --git a/gold/layout.cc b/gold/layout.cc
index a50086897bb..248844018dc 100644
--- a/gold/layout.cc
+++ b/gold/layout.cc
@@ -2918,17 +2918,22 @@ Layout::find_section_order_index(const std::string& section_name)
 // Read the sequence of input sections from the file specified with
 // option --section-ordering-file.
 
-void
+Exit_status
 Layout::read_layout_from_file()
 {
-  const char* filename = parameters->options().section_ordering_file();
+  const char* filename = parameters->get_section_ordering_file();
+  bool mayfail = parameters->section_ordering_file_mayfail();
   std::ifstream in;
   std::string line;
 
   in.open(filename);
-  if (!in)
-    gold_fatal(_("unable to open --section-ordering-file file %s: %s"),
-	       filename, strerror(errno));
+  if (!in) {
+      if(mayfail)
+          gold_fatal(_("unable to open --section-ordering-file file %s: %s"),
+                     filename, strerror(errno));
+      else
+          return GOLD_ERR;
+  }
 
   File_read::record_file_read(filename);
 
@@ -2953,6 +2958,8 @@ Layout::read_layout_from_file()
       position++;
       std::getline(in, line);
     }
+
+  return GOLD_OK;
 }
 
 // Finalize the layout.  When this is called, we have created all the
diff --git a/gold/layout.h b/gold/layout.h
index 5bb9aff5b04..bcfd2e871c4 100644
--- a/gold/layout.h
+++ b/gold/layout.h
@@ -617,7 +617,7 @@ class Layout
 
   // Read the sequence of input sections from the file specified with
   // linker option --section-ordering-file.
-  void
+  Exit_status
   read_layout_from_file();
 
   // Layout an input reloc section when doing a relocatable link.  The
diff --git a/gold/main.cc b/gold/main.cc
index c6aa1901a95..404aad2085b 100644
--- a/gold/main.cc
+++ b/gold/main.cc
@@ -174,6 +174,8 @@ main(int argc, char** argv)
   // Store some options in the globally accessible parameters.
   set_parameters_options(&command_line.options());
 
+  set_parameters_section_ordering_file_from_env();
+
   // Do this as early as possible (since it prints a welcome message).
   write_debug_script(command_line.options().output_file_name(),
                      program_name, args.c_str());
@@ -232,8 +234,9 @@ main(int argc, char** argv)
   if (layout.incremental_inputs() != NULL)
     layout.incremental_inputs()->report_command_line(argc, argv);
 
-  if (parameters->options().section_ordering_file())
-    layout.read_layout_from_file();
+  if (parameters->get_section_ordering_file())
+    if (layout.read_layout_from_file() == GOLD_ERR)
+      set_parameters_section_ordering_file_failure();
 
   // Load plugin libraries.
   if (command_line.options().has_plugins())
diff --git a/gold/options.h b/gold/options.h
index 46f658f23ea..bc9bdc5ed13 100644
--- a/gold/options.h
+++ b/gold/options.h
@@ -1243,7 +1243,9 @@ class General_options
 	      N_("Strip LTO intermediate code sections"), NULL);
 
   DEFINE_string(section_ordering_file, options::TWO_DASHES, '\0', NULL,
-		N_("Layout sections in the order specified"),
+		N_("Layout sections in the order specified. May also be "
+		   "specified using the environment variable "
+		   "'GLOBAL_SECTION_ORDERING_FILE'"),
 		N_("FILENAME"));
 
   DEFINE_special(section_start, options::TWO_DASHES, '\0',
diff --git a/gold/output.cc b/gold/output.cc
index a1978eb5f32..31be6270680 100644
--- a/gold/output.cc
+++ b/gold/output.cc
@@ -2532,7 +2532,7 @@ Output_section::add_input_section(Layout* layout,
       /* If section ordering is requested by specifying a ordering file,
 	 using --section-ordering-file, match the section name with
 	 a pattern.  */
-      if (parameters->options().section_ordering_file())
+      if (parameters->get_section_ordering_file())
 	{
 	  unsigned int section_order_index =
 	    layout->find_section_order_index(std::string(secname));
diff --git a/gold/parameters.cc b/gold/parameters.cc
index 90bc4db8d46..60d97e66abf 100644
--- a/gold/parameters.cc
+++ b/gold/parameters.cc
@@ -231,6 +231,49 @@ Parameters::check_rodata_segment()
     gold_error(_("-Trodata-segment is meaningless without --rosegment"));
 }
 
+// Set the section_ordering_file using 'GLOBAL_SECTION_ORDERING_FILE' if the
+// user-option '--section-ordering-file' wasn't specified.
+void
+Parameters::set_section_ordering_file_from_env()
+{
+  const char *ret;
+  // If we have user-option or already had a failure, nothing to do.
+  if (this->options().section_ordering_file()
+  || this->section_ordering_file_has_failed_)
+    return;
+
+  // Read filepath from environment.
+  ret = getenv("GLOBAL_SECTION_ORDERING_FILE");
+
+  // If not set (or error), indicate we should stop trying to return.
+  if (ret == NULL)
+    {
+      this->section_ordering_file_has_failed_ = true;
+      return;
+    }
+
+  // Set env file.
+  section_ordering_file_from_env_ = ret;
+}
+
+// Return section_ordering_file, first checking user-option then checking env
+// variable input.
+const char *
+Parameters::get_section_ordering_file() const
+{
+  // If we have user-option return it.
+  const char *ret = this->options().section_ordering_file();
+  if (ret)
+    return ret;
+
+  // If we had any error using the env file then pretend it never existed.
+  if (this->section_ordering_file_has_failed_)
+    return NULL;
+
+  // Finally return env file. This may be NULL if it was never specified.
+  return this->section_ordering_file_from_env_;
+}
+
 // Return the name of the entry symbol.
 
 const char*
@@ -282,6 +325,14 @@ Parameters::incremental_update() const
 	  || this->incremental_mode_ == General_options::INCREMENTAL_AUTO);
 }
 
+void
+set_parameters_section_ordering_file_failure()
+{ static_parameters.set_section_ordering_file_failure(); }
+
+void
+set_parameters_section_ordering_file_from_env()
+{ static_parameters.set_section_ordering_file_from_env(); }
+
 void
 set_parameters_errors(Errors* errors)
 { static_parameters.set_errors(errors); }
diff --git a/gold/parameters.h b/gold/parameters.h
index 76202b186e4..e81506eaa14 100644
--- a/gold/parameters.h
+++ b/gold/parameters.h
@@ -176,7 +176,36 @@ class Parameters
   bool
   incremental_update() const;
 
- private:
+  // Get section_ordering_file. This first tries the user-option from
+  // '--section-ordering-file'. It ifs not present, it tries the file from the
+  // env variable 'GLOBAL_SECTION_ORDERING_FILE'.
+  const char *get_section_ordering_file() const;
+
+  // Set that we should throw error if we have IO issues with the
+  // section_order_file. It we are using the user-provided argument we throw
+  // error in the event of IO issue. If we are using the env version we ignore
+  // IO errors and stop using the section_ordering_file.
+  bool
+  section_ordering_file_mayfail() const
+  {
+    return this->section_ordering_file_from_env_ == NULL;
+  }
+
+  // Only relevent if we are using the env variable section_ordering_file. It
+  // indicates we failed doing IO on the file, so we want to pretend it never
+  // existed.
+  void
+  set_section_ordering_file_failure()
+  {
+    this->section_ordering_file_has_failed_ = true;
+  }
+
+  // Read environment variable 'GLOBAL_SECTION_ORDERING_FILE' and, if present,
+  // use its value for the section_ordering_file. Only relevent of user-option
+  // '--section-ordering-file' is not present.
+  void set_section_ordering_file_from_env();
+
+private:
   void
   set_target_once(Target*);
 
@@ -197,6 +226,8 @@ class Parameters
   int debug_;
   int incremental_mode_;
   Set_parameters_target_once* set_parameters_target_once_;
+  const char *section_ordering_file_from_env_ = NULL;
+  bool section_ordering_file_has_failed_ = false;
 };
 
 // This is a global variable.
@@ -205,6 +236,12 @@ extern const Parameters* parameters;
 // We use free functions for these since they affect a global variable
 // that is internal to parameters.cc.
 
+extern void
+set_parameters_section_ordering_file_failure();
+
+extern void
+set_parameters_section_ordering_file_from_env();
+
 extern void
 set_parameters_errors(Errors* errors);
 
-- 
2.34.1

Re: [PATCH v1] gold: Make '--section-ordering-file' also specifiable in env variable

[Fangrui Song]

On Thu, May 11, 2023 at 2:02 PM Noah Goldstein via Binutils
<binutils@sourceware.org> wrote:
>
> New proposed env variable is 'GLOBAL_SECTION_ORDERING_FILE'.
>
> The motivation for this change to help package maintainers use the
> section ordering feature. For package maintainers, modifying the link
> flags of all their maintained projects, is time consumining to a
> degree that it is often considered infeasable. By making the feature
> enablable simply with an environment variable, removes this
> roadblock. Similiarly, the rationale behind ignoring errors when using
> the environment specified file is to make it so that it never create
> new problems in the build process.
>
> The hope by making this feature more accessable it will allow package
> maintainers to use ordering lists from profiles of their applications
> to distribute better optimized packages.

The motivation isn't compelling to me. Using an environment variable
to control a linker option isn't a convention.
I probably should say, it's very odd, going against the spirit of a linker.

Can you use LDFLAGS or a linker wrapper to achieve your goal?

% cat ld.gold
#!/bin/sh -e
...


If some packages don't recognize standard variables CFLAGS/LDFLAGS,
just help add them so that other distributions can control global
compiler/linker options more conveniently.

Re: [PATCH v1] gold: Make '--section-ordering-file' also specifiable in env variable

[Noah Goldstein]

On Fri, May 12, 2023 at 1:48 AM Fangrui Song <i@maskray.me> wrote:
>
> On Thu, May 11, 2023 at 2:02 PM Noah Goldstein via Binutils
> <binutils@sourceware.org> wrote:
> >
> > New proposed env variable is 'GLOBAL_SECTION_ORDERING_FILE'.
> >
> > The motivation for this change to help package maintainers use the
> > section ordering feature. For package maintainers, modifying the link
> > flags of all their maintained projects, is time consumining to a
> > degree that it is often considered infeasable. By making the feature
> > enablable simply with an environment variable, removes this
> > roadblock. Similiarly, the rationale behind ignoring errors when using
> > the environment specified file is to make it so that it never create
> > new problems in the build process.
> >
> > The hope by making this feature more accessable it will allow package
> > maintainers to use ordering lists from profiles of their applications
> > to distribute better optimized packages.
>
> The motivation isn't compelling to me. Using an environment variable
> to control a linker option isn't a convention.
> I probably should say, it's very odd, going against the spirit of a linker.
>
> Can you use LDFLAGS or a linker wrapper to achieve your goal?
>
> % cat ld.gold
> #!/bin/sh -e
> ...
>
>
> If some packages don't recognize standard variables CFLAGS/LDFLAGS,
> just help add them so that other distributions can control global
> compiler/linker options more conveniently.

Okay, would you be okay with a new commandline flag to just not fail
linking if there are issues with the section ordering file? Something
like "--section-ordering-ignore-errors"?

Re: [PATCH v1] gold: Make '--section-ordering-file' also specifiable in env variable

[Fangrui Song]

On Fri, May 12, 2023 at 9:09 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Fri, May 12, 2023 at 1:48 AM Fangrui Song <i@maskray.me> wrote:
> >
> > On Thu, May 11, 2023 at 2:02 PM Noah Goldstein via Binutils
> > <binutils@sourceware.org> wrote:
> > >
> > > New proposed env variable is 'GLOBAL_SECTION_ORDERING_FILE'.
> > >
> > > The motivation for this change to help package maintainers use the
> > > section ordering feature. For package maintainers, modifying the link
> > > flags of all their maintained projects, is time consumining to a
> > > degree that it is often considered infeasable. By making the feature
> > > enablable simply with an environment variable, removes this
> > > roadblock. Similiarly, the rationale behind ignoring errors when using
> > > the environment specified file is to make it so that it never create
> > > new problems in the build process.
> > >
> > > The hope by making this feature more accessable it will allow package
> > > maintainers to use ordering lists from profiles of their applications
> > > to distribute better optimized packages.
> >
> > The motivation isn't compelling to me. Using an environment variable
> > to control a linker option isn't a convention.
> > I probably should say, it's very odd, going against the spirit of a linker.
> >
> > Can you use LDFLAGS or a linker wrapper to achieve your goal?
> >
> > % cat ld.gold
> > #!/bin/sh -e
> > ...
> >
> >
> > If some packages don't recognize standard variables CFLAGS/LDFLAGS,
> > just help add them so that other distributions can control global
> > compiler/linker options more conveniently.
>
> Okay, would you be okay with a new commandline flag to just not fail
> linking if there are issues with the section ordering file? Something
> like "--section-ordering-ignore-errors"?

When are there issues with the section ordering file?
For a pattern that doesn't match any input section, there is no linker error.
See my example at
https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#:~:text=section%2dordering%2dfile
and my description of why --symbol-ordering-file= may be more useful.

I think our discussion may be a case of https://xyproblem.info/ . If
you describe your original problem, I or someone may give a better
answer.