public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] plugins: Allow plugins to handle global_options changes
@ 2020-11-18  9:13 Jakub Jelinek
  2020-11-18  9:39 ` Richard Biener
  0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2020-11-18  9:13 UTC (permalink / raw)
  To: gcc-patches

Hi!

Reposting with self-contained description per Joseph's request:

Any time somebody adds or removes an option in some *.opt file (which e.g.
on the 10 branch after branching off 11 happened 7 times already), many
offsets in global_options variable change and so plugins that ever access
GCC options or other global_options values are ABI dependent on it.  It is
true we don't guarantee ABI stability for plugins, but we change the most
often used data structures on the release branches only very rarely and so
the options changes are the most problematic for ABI stability of plugins.

Annobin uses a way to remap accesses to some of the global_options.x_* by
looking them up in the cl_options array where we have
offsetof (struct gcc_options, x_flag_lto)
etc. remembered, but sadly doesn't do it for all options (e.g. some flag_*
etc. option accesses may be hidden in various macros like POINTER_SIZE),
and more importantly some struct gcc_options offsets are not covered at all.
E.g. there is no offsetof (struct gcc_options, x_optimize),
offsetof (struct gcc_options, x_flag_sanitize) etc.  Those are usually:
Variable
int optimize
in the *.opt files.

The following patch allows the plugins to deal with reshuffling of even
the global_options fields that aren't tracked in cl_options by adding
another array that describes those, which adds an 816 bytes long array
and 1039 bytes in string literals, so 1855 .rodata bytes in total ATM.

If needed, this could be guarded by some configure option if those 1855
.rodata (0.02% of .rodata size) bytes is something that people don't want
to sacrifice for this.

Bootstrapped/regtested again last night on x86_64-linux and i686-linux, ok
for trunk?

Or if not, would it be ok if this was guarded by some
--enable-plugin-option-tracking
or other configure option?

2020-11-18  Jakub Jelinek  <jakub@redhat.com>

	* opts.h (struct cl_var): New type.
	(cl_vars): Declare.
	* optc-gen.awk: Generate cl_vars array.

--- gcc/opts.h.jj	2020-04-30 11:49:28.462900760 +0200
+++ gcc/opts.h	2020-08-24 10:21:08.563288412 +0200
@@ -124,6 +124,14 @@ struct cl_option
   int range_max;
 };
 
+struct cl_var
+{
+  /* Name of the variable.  */
+  const char *var_name;
+  /* Offset of field for this var in struct gcc_options.  */
+  unsigned short var_offset;
+};
+
 /* Records that the state of an option consists of SIZE bytes starting
    at DATA.  DATA might point to CH in some cases.  */
 struct cl_option_state {
@@ -134,6 +142,7 @@ struct cl_option_state {
 
 extern const struct cl_option cl_options[];
 extern const unsigned int cl_options_count;
+extern const struct cl_var cl_vars[];
 extern const char *const lang_names[];
 extern const unsigned int cl_lang_count;
 
--- gcc/optc-gen.awk.jj	2020-01-12 11:54:36.691409214 +0100
+++ gcc/optc-gen.awk	2020-08-24 10:19:49.410410288 +0200
@@ -592,5 +592,29 @@ for (i = 0; i < n_opts; i++) {
 }
 print "}               "
 
+split("", var_seen, ":")
+print "\n#ifndef GENERATOR_FILE"
+print "DEBUG_VARIABLE const struct cl_var cl_vars[] =\n{"
+
+for (i = 0; i < n_opts; i++) {
+	name = var_name(flags[i]);
+	if (name == "")
+		continue;
+	var_seen[name] = 1;
+}
+
+for (i = 0; i < n_extra_vars; i++) {
+	var = extra_vars[i]
+	sub(" *=.*", "", var)
+	name = var
+	sub("^.*[ *]", "", name)
+	sub("\\[.*\\]$", "", name)
+	if (name in var_seen)
+		continue;
+	print "  { " quote name quote ", offsetof (struct gcc_options, x_" name ") },"
+	var_seen[name] = 1
 }
 
+print "  { NULL, (unsigned short) -1 }\n};\n#endif"
+
+}

	Jakub


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] plugins: Allow plugins to handle global_options changes
  2020-11-18  9:13 [PATCH] plugins: Allow plugins to handle global_options changes Jakub Jelinek
@ 2020-11-18  9:39 ` Richard Biener
  2020-11-18  9:47   ` Jakub Jelinek
  0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2020-11-18  9:39 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC Patches

On Wed, Nov 18, 2020 at 10:14 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi!
>
> Reposting with self-contained description per Joseph's request:
>
> Any time somebody adds or removes an option in some *.opt file (which e.g.
> on the 10 branch after branching off 11 happened 7 times already), many
> offsets in global_options variable change and so plugins that ever access
> GCC options or other global_options values are ABI dependent on it.  It is
> true we don't guarantee ABI stability for plugins, but we change the most
> often used data structures on the release branches only very rarely and so
> the options changes are the most problematic for ABI stability of plugins.
>
> Annobin uses a way to remap accesses to some of the global_options.x_* by
> looking them up in the cl_options array where we have
> offsetof (struct gcc_options, x_flag_lto)
> etc. remembered, but sadly doesn't do it for all options (e.g. some flag_*
> etc. option accesses may be hidden in various macros like POINTER_SIZE),
> and more importantly some struct gcc_options offsets are not covered at all.
> E.g. there is no offsetof (struct gcc_options, x_optimize),
> offsetof (struct gcc_options, x_flag_sanitize) etc.  Those are usually:
> Variable
> int optimize
> in the *.opt files.
>
> The following patch allows the plugins to deal with reshuffling of even
> the global_options fields that aren't tracked in cl_options by adding
> another array that describes those, which adds an 816 bytes long array
> and 1039 bytes in string literals, so 1855 .rodata bytes in total ATM.
>
> If needed, this could be guarded by some configure option if those 1855
> .rodata (0.02% of .rodata size) bytes is something that people don't want
> to sacrifice for this.
>
> Bootstrapped/regtested again last night on x86_64-linux and i686-linux, ok
> for trunk?
>
> Or if not, would it be ok if this was guarded by some
> --enable-plugin-option-tracking
> or other configure option?

We already have --{enable,disable}-plugin, so could remove it when
those are not enabled.  OTOH the GCC binaries are already gigantic in size.

Richard.

> 2020-11-18  Jakub Jelinek  <jakub@redhat.com>
>
>         * opts.h (struct cl_var): New type.
>         (cl_vars): Declare.
>         * optc-gen.awk: Generate cl_vars array.
>
> --- gcc/opts.h.jj       2020-04-30 11:49:28.462900760 +0200
> +++ gcc/opts.h  2020-08-24 10:21:08.563288412 +0200
> @@ -124,6 +124,14 @@ struct cl_option
>    int range_max;
>  };
>
> +struct cl_var
> +{
> +  /* Name of the variable.  */
> +  const char *var_name;
> +  /* Offset of field for this var in struct gcc_options.  */
> +  unsigned short var_offset;
> +};
> +
>  /* Records that the state of an option consists of SIZE bytes starting
>     at DATA.  DATA might point to CH in some cases.  */
>  struct cl_option_state {
> @@ -134,6 +142,7 @@ struct cl_option_state {
>
>  extern const struct cl_option cl_options[];
>  extern const unsigned int cl_options_count;
> +extern const struct cl_var cl_vars[];
>  extern const char *const lang_names[];
>  extern const unsigned int cl_lang_count;
>
> --- gcc/optc-gen.awk.jj 2020-01-12 11:54:36.691409214 +0100
> +++ gcc/optc-gen.awk    2020-08-24 10:19:49.410410288 +0200
> @@ -592,5 +592,29 @@ for (i = 0; i < n_opts; i++) {
>  }
>  print "}               "
>
> +split("", var_seen, ":")
> +print "\n#ifndef GENERATOR_FILE"
> +print "DEBUG_VARIABLE const struct cl_var cl_vars[] =\n{"
> +
> +for (i = 0; i < n_opts; i++) {
> +       name = var_name(flags[i]);
> +       if (name == "")
> +               continue;
> +       var_seen[name] = 1;
> +}
> +
> +for (i = 0; i < n_extra_vars; i++) {
> +       var = extra_vars[i]
> +       sub(" *=.*", "", var)
> +       name = var
> +       sub("^.*[ *]", "", name)
> +       sub("\\[.*\\]$", "", name)
> +       if (name in var_seen)
> +               continue;
> +       print "  { " quote name quote ", offsetof (struct gcc_options, x_" name ") },"
> +       var_seen[name] = 1
>  }
>
> +print "  { NULL, (unsigned short) -1 }\n};\n#endif"
> +
> +}
>
>         Jakub
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] plugins: Allow plugins to handle global_options changes
  2020-11-18  9:39 ` Richard Biener
@ 2020-11-18  9:47   ` Jakub Jelinek
  2020-11-18 18:47     ` Joseph Myers
  0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2020-11-18  9:47 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches

On Wed, Nov 18, 2020 at 10:39:46AM +0100, Richard Biener wrote:
> We already have --{enable,disable}-plugin, so could remove it when
> those are not enabled.

Here is a variant that does that:

2020-11-18  Jakub Jelinek  <jakub@redhat.com>

	* opts.h (struct cl_var): New type.
	(cl_vars): Declare.
	* optc-gen.awk: Generate cl_vars array.

--- gcc/opts.h.jj	2020-04-30 11:49:28.462900760 +0200
+++ gcc/opts.h	2020-08-24 10:21:08.563288412 +0200
@@ -124,6 +124,14 @@ struct cl_option
   int range_max;
 };
 
+struct cl_var
+{
+  /* Name of the variable.  */
+  const char *var_name;
+  /* Offset of field for this var in struct gcc_options.  */
+  unsigned short var_offset;
+};
+
 /* Records that the state of an option consists of SIZE bytes starting
    at DATA.  DATA might point to CH in some cases.  */
 struct cl_option_state {
@@ -134,6 +142,9 @@ struct cl_option_state {
 
 extern const struct cl_option cl_options[];
 extern const unsigned int cl_options_count;
+#ifdef ENABLE_PLUGIN
+extern const struct cl_var cl_vars[];
+#endif
 extern const char *const lang_names[];
 extern const unsigned int cl_lang_count;
 
--- gcc/optc-gen.awk.jj	2020-01-12 11:54:36.691409214 +0100
+++ gcc/optc-gen.awk	2020-08-24 10:19:49.410410288 +0200
@@ -592,5 +592,29 @@ for (i = 0; i < n_opts; i++) {
 }
 print "}               "
 
+split("", var_seen, ":")
+print "\n#if !defined(GENERATOR_FILE) && defined(ENABLE_PLUGIN)"
+print "DEBUG_VARIABLE const struct cl_var cl_vars[] =\n{"
+
+for (i = 0; i < n_opts; i++) {
+	name = var_name(flags[i]);
+	if (name == "")
+		continue;
+	var_seen[name] = 1;
+}
+
+for (i = 0; i < n_extra_vars; i++) {
+	var = extra_vars[i]
+	sub(" *=.*", "", var)
+	name = var
+	sub("^.*[ *]", "", name)
+	sub("\\[.*\\]$", "", name)
+	if (name in var_seen)
+		continue;
+	print "  { " quote name quote ", offsetof (struct gcc_options, x_" name ") },"
+	var_seen[name] = 1
 }
 
+print "  { NULL, (unsigned short) -1 }\n};\n#endif"
+
+}

	Jakub


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] plugins: Allow plugins to handle global_options changes
  2020-11-18  9:47   ` Jakub Jelinek
@ 2020-11-18 18:47     ` Joseph Myers
  0 siblings, 0 replies; 4+ messages in thread
From: Joseph Myers @ 2020-11-18 18:47 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Richard Biener, GCC Patches

On Wed, 18 Nov 2020, Jakub Jelinek via Gcc-patches wrote:

> On Wed, Nov 18, 2020 at 10:39:46AM +0100, Richard Biener wrote:
> > We already have --{enable,disable}-plugin, so could remove it when
> > those are not enabled.
> 
> Here is a variant that does that:
> 
> 2020-11-18  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* opts.h (struct cl_var): New type.
> 	(cl_vars): Declare.
> 	* optc-gen.awk: Generate cl_vars array.

This version is OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-11-18 18:47 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18  9:13 [PATCH] plugins: Allow plugins to handle global_options changes Jakub Jelinek
2020-11-18  9:39 ` Richard Biener
2020-11-18  9:47   ` Jakub Jelinek
2020-11-18 18:47     ` Joseph Myers

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