[v1] gold: Make '--section-ordering-file' also specifiable in env variable
Checks
Commit Message
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(-)
Comments
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.
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"?
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.
@@ -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();
@@ -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
@@ -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
@@ -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())
@@ -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',
@@ -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));
@@ -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); }
@@ -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);