[gold patch] Change option registration

[Cary Coutant <ccoutant at google dot com>]

While fixing the -s problem, I wanted to add an assert to
One_option::register_option that would assure we never accidentally
created two options with the same short version again. Unfortunately,
it wasn't quite that simple, since gold actually registers all of its
options twice -- once as part of the static constructor for
Position_dependent_options::default_options_, and again when
Command_line is instantiated in main(). We really only want to
register the options for the second of those instantiations of
General_options.

Given that, I wondered why --help only printed the options once, since
they all were registered twice. It turns out that in options.cc, the
registered_options vector gets constructed AFTER
Position_dependent_options::default_options_, so all the options that
got pushed onto that vector during the first round of registration got
tossed aside when the vector got constructed. So as it turns out, we
were pushing the first set of options onto an unconstructed vector.

The attached patch adds a flag that controls whether or not we want to
register the options. I use a dummy member class in class
Command_line, and have that class' constructor set the flag to true. I
rely on constructor order within the class to make sure the dummy
class' constructor runs before the constructor for options_. When the
static constructors run for
Position_dependent_options::default_options_, the flag is false, and
we do not register any options. When construction of General_options
is complete, I reset the flag so we don't depend on which top-level
object gets constructed first. (A more elegant solution might have
been to templatize class General_options with a boolean constant, but
that seemed far too invasive for what little gain this patch
provides.)

I think this change would also let us statically allocate the
long_options map, since we now are assured that registration happens
only during the execution of main(), where the Command_line object is
allocated:

  // We can't make long_options a static Option_map because we can't
  // guarantee that will be initialized before register_option() is
  // first called.
  if (long_options == NULL)
    long_options = new Option_map;

Comments?

-cary


	* options.h (Command_line::Pre_options): New class.
	(Command_line::pre_options): New member.
	* options.cc (gold::options::ready_to_register): New variable.
	(One_option::register_option): Do nothing if not registering options.
	Assert if same short option registered twice.
	(General_options::General_options): Turn off option registration when
	done constructing.
	(Command_line::Pre_options::Pre_options): New constructor.


Index: options.cc
===================================================================
RCS file: /cvs/src/src/gold/options.cc,v
retrieving revision 1.88
diff -u -p -r1.88 options.cc
--- options.cc	24 Jun 2009 05:01:17 -0000	1.88
+++ options.cc	24 Aug 2009 23:48:59 -0000
@@ -47,6 +47,11 @@ Position_dependent_options::default_opti
 namespace options
 {

+// This flag is TRUE if we should register the command-line options as they
+// are constructed.  It is set after contruction of the options within
+// class Position_dependent_options.
+static bool ready_to_register = false;
+
 // This global variable is set up as General_options is constructed.
 static std::vector<const One_option*> registered_options;

@@ -60,6 +65,9 @@ static One_option* short_options[128];
 void
 One_option::register_option()
 {
+  if (!ready_to_register)
+    return;
+
   registered_options.push_back(this);

   // We can't make long_options a static Option_map because we can't
@@ -75,7 +83,10 @@ One_option::register_option()
   const int shortname_as_int = static_cast<int>(this->shortname);
   gold_assert(shortname_as_int >= 0 && shortname_as_int < 128);
   if (this->shortname != '\0')
-    short_options[shortname_as_int] = this;
+    {
+      gold_assert(short_options[shortname_as_int] == NULL);
+      short_options[shortname_as_int] = this;
+    }
 }

 void
@@ -714,6 +725,8 @@ General_options::General_options()
     do_demangle_(false), plugins_(),
     incremental_disposition_(INCREMENTAL_CHECK), implicit_incremental_(false)
 {
+  // Turn off option registration once construction is complete.
+  gold::options::ready_to_register = false;
 }

 General_options::Object_format
@@ -1039,6 +1052,13 @@ Command_line::Command_line()
 {
 }

+// Pre_options is the hook that sets the ready_to_register flag.
+
+Command_line::Pre_options::Pre_options()
+{
+  gold::options::ready_to_register = true;
+}
+
 // Process the command line options.  For process_one_option, i is the
 // index of argv to process next, and must be an option (that is,
 // start with a dash).  The return value is the index of the next
Index: options.h
===================================================================
RCS file: /cvs/src/src/gold/options.h,v
retrieving revision 1.105
diff -u -p -r1.105 options.h
--- options.h	24 Aug 2009 23:31:45 -0000	1.105
+++ options.h	24 Aug 2009 23:48:59 -0000
@@ -1479,6 +1479,16 @@ class Command_line
   Command_line(const Command_line&);
   Command_line& operator=(const Command_line&);

+  // This is a dummy class to provide a constructor that runs before
+  // the constructor for the General_options.  The Pre_options constructor
+  // is used as a hook to set the flag enabling the options to register
+  // themselves.
+  struct Pre_options {
+    Pre_options();
+  };
+
+  // This must come before options_!
+  Pre_options pre_options_;
   General_options options_;
   Position_dependent_options position_options_;
   Script_options script_options_;