Provide string description of definition, visibility and resolution in LTO plug-in.

Message ID c6183e18-1908-aaff-d2fb-ad30b09042a8@suse.cz
State New
Headers show
Series
  • Provide string description of definition, visibility and resolution in LTO plug-in.
Related show

Commit Message

Martin Liška Feb. 26, 2019, 2:41 p.m.
Hi.

I would like to enhance get_symbols function to report string
representation of definition, visibility and resolution.
It's easier for users to read it.

Patch survives tests on x86_64-linux-gnu.

ld/ChangeLog:

2019-02-26  Martin Liska  <mliska@suse.cz>

	* plugin.c (get_symbols): Add lto_kind_str, lto_resolution_str,
	lto_visibility_str and use then to inform about plugin-symbols.
	* testsuite/ld-plugin/plugin-12.d: Adjust expected pattern.
---
 ld/plugin.c                        | 37 ++++++++++++++++++++++++++++--
 ld/testsuite/ld-plugin/plugin-12.d |  8 +++----
 2 files changed, 39 insertions(+), 6 deletions(-)

Comments

Martin Liška March 8, 2019, 10:36 a.m. | #1
PING^1

On 2/26/19 3:41 PM, Martin Liška wrote:
> Hi.

> 

> I would like to enhance get_symbols function to report string

> representation of definition, visibility and resolution.

> It's easier for users to read it.

> 

> Patch survives tests on x86_64-linux-gnu.

> 

> ld/ChangeLog:

> 

> 2019-02-26  Martin Liska  <mliska@suse.cz>

> 

> 	* plugin.c (get_symbols): Add lto_kind_str, lto_resolution_str,

> 	lto_visibility_str and use then to inform about plugin-symbols.

> 	* testsuite/ld-plugin/plugin-12.d: Adjust expected pattern.

> ---

>  ld/plugin.c                        | 37 ++++++++++++++++++++++++++++--

>  ld/testsuite/ld-plugin/plugin-12.d |  8 +++----

>  2 files changed, 39 insertions(+), 6 deletions(-)

> 

>
Nick Clifton March 13, 2019, 12:28 p.m. | #2
Hi Martin,
 
  Sorry for the delay in reviewing.

> I would like to enhance get_symbols function to report string

> representation of definition, visibility and resolution.

> It's easier for users to read it.


Agreed, but ...

@@ -777,9 +808,11 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
       syms[n].resolution = res;
       if (report_plugin_symbols)
 	einfo (_("%P: %pB: symbol `%s' "
-		 "definition: %d, visibility: %d, resolution: %d\n"),
+		 "definition: %s, visibility: %s, resolution: %s\n"),
 	       abfd, syms[n].name,
-	       syms[n].def, syms[n].visibility, res);
+	       lto_kind_str[syms[n].def],
+	       lto_visibility_str[syms[n].visibility],
+	       lto_resolution_str[res]);

You need to be more paranoid.  If one of the values is not in the
expected range, then you will have an illegal or unexpected memory
dereference via the array accesses.

My suggestion - create a set of small new functions which return
the name of a particular value, and which handle unknown values.
eg:

  const char *
  get_lto_kind (unsigned int index)
  {
     const char *lto_kind_str[5] =
     {
      "DEF",
      "WEAKDEF",
      "UNDEF",
      "WEAKUNDEF",
      "COMMON"
     };

    if (index < ARRAY_SIZE (lto_kind_str))
      return lto_kind_str [index];

    const char buf [1024];
    sprintf (buf, _("unknown definition value %x"), index);
    return buf;
  }

Then use these new functions in the einfo() statement.

Cheers
  Nick
Martin Liška March 14, 2019, 9:09 a.m. | #3
On 3/13/19 1:28 PM, Nick Clifton wrote:
> Hi Martin,

>  

>   Sorry for the delay in reviewing.


Hi.

That's fine.

> 

>> I would like to enhance get_symbols function to report string

>> representation of definition, visibility and resolution.

>> It's easier for users to read it.

> 

> Agreed, but ...

> 

> @@ -777,9 +808,11 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,

>        syms[n].resolution = res;

>        if (report_plugin_symbols)

>  	einfo (_("%P: %pB: symbol `%s' "

> -		 "definition: %d, visibility: %d, resolution: %d\n"),

> +		 "definition: %s, visibility: %s, resolution: %s\n"),

>  	       abfd, syms[n].name,

> -	       syms[n].def, syms[n].visibility, res);

> +	       lto_kind_str[syms[n].def],

> +	       lto_visibility_str[syms[n].visibility],

> +	       lto_resolution_str[res]);

> 

> You need to be more paranoid.  If one of the values is not in the

> expected range, then you will have an illegal or unexpected memory

> dereference via the array accesses.

> 

> My suggestion - create a set of small new functions which return

> the name of a particular value, and which handle unknown values.

> eg:

> 

>   const char *

>   get_lto_kind (unsigned int index)

>   {

>      const char *lto_kind_str[5] =

>      {

>       "DEF",

>       "WEAKDEF",

>       "UNDEF",

>       "WEAKUNDEF",

>       "COMMON"

>      };

> 

>     if (index < ARRAY_SIZE (lto_kind_str))

>       return lto_kind_str [index];

> 

>     const char buf [1024];

>     sprintf (buf, _("unknown definition value %x"), index);

>     return buf;

>   }

> 

> Then use these new functions in the einfo() statement.

> 

> Cheers

>   Nick

> 


Good point, I reworked that.

Patch survives tests on x86_64-linux-gnu.

Martin
From e6fda49aa3d7ad69f8034fd10bfa876e15fd3b8b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>

Date: Tue, 26 Feb 2019 15:19:12 +0100
Subject: [PATCH] Provide string description of definition, visibility and
 resolution in LTO plug-in.

ld/ChangeLog:

2019-02-26  Martin Liska  <mliska@suse.cz>

	* plugin.c (get_symbols): Add lto_kind_str, lto_resolution_str,
	lto_visibility_str and use then to inform about plugin-symbols.
	* testsuite/ld-plugin/plugin-12.d: Adjust expected pattern.
---
 ld/plugin.c                        | 73 +++++++++++++++++++++++++++++-
 ld/testsuite/ld-plugin/plugin-12.d |  8 ++--
 2 files changed, 75 insertions(+), 6 deletions(-)

diff --git a/ld/plugin.c b/ld/plugin.c
index ea1a7f7064..fd3dcbc8d3 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -659,6 +659,73 @@ is_visible_from_outside (struct ld_plugin_symbol *lsym,
   return FALSE;
 }
 
+/* Return LTO kind string name that corresponds to INDEX enum value.  */
+static const char *
+get_lto_kind (unsigned int index)
+{
+  const char *lto_kind_str[5] =
+  {
+    "DEF",
+    "WEAKDEF",
+    "UNDEF",
+    "WEAKUNDEF",
+    "COMMON"
+  };
+
+  if (index < ARRAY_SIZE (lto_kind_str))
+    return lto_kind_str [index];
+
+  char *buf = xmalloc (64);
+  sprintf (buf, _("unknown LTO kind value %x"), index);
+  return buf;
+}
+
+/* Return LTO resolution string name that corresponds to INDEX enum value.  */
+static const char *
+get_lto_resolution (unsigned int index)
+{
+  static const char *lto_resolution_str[10] =
+  {
+    "UNKNOWN",
+    "UNDEF",
+    "PREVAILING_DEF",
+    "PREVAILING_DEF_IRONLY",
+    "PREEMPTED_REG",
+    "PREEMPTED_IR",
+    "RESOLVED_IR",
+    "RESOLVED_EXEC",
+    "RESOLVED_DYN",
+    "PREVAILING_DEF_IRONLY_EXP",
+  };
+
+  if (index < ARRAY_SIZE (lto_resolution_str))
+    return lto_resolution_str [index];
+
+  char *buf = xmalloc (64);
+  sprintf (buf, _("unknown LTO resolution value %x"), index);
+  return buf;
+}
+
+/* Return LTO visibility string name that corresponds to INDEX enum value.  */
+static const char *
+get_lto_visibility (unsigned int index)
+{
+  const char *lto_visibility_str[4] =
+  {
+    "DEFAULT",
+    "PROTECTED",
+    "INTERNAL",
+    "HIDDEN"
+  };
+
+  if (index < ARRAY_SIZE (lto_visibility_str))
+    return lto_visibility_str [index];
+
+  char *buf = xmalloc (64);
+  sprintf (buf, _("unknown LTO visibility value %x"), index);
+  return buf;
+}
+
 /* Get the symbol resolution info for a plugin-claimed input file.  */
 static enum ld_plugin_status
 get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
@@ -777,9 +844,11 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
       syms[n].resolution = res;
       if (report_plugin_symbols)
 	einfo (_("%P: %pB: symbol `%s' "
-		 "definition: %d, visibility: %d, resolution: %d\n"),
+		 "definition: %s, visibility: %s, resolution: %s\n"),
 	       abfd, syms[n].name,
-	       syms[n].def, syms[n].visibility, res);
+	       get_lto_kind (syms[n].def),
+	       get_lto_visibility (syms[n].visibility),
+	       get_lto_resolution (res));
     }
   return LDPS_OK;
 }
diff --git a/ld/testsuite/ld-plugin/plugin-12.d b/ld/testsuite/ld-plugin/plugin-12.d
index 10d772553d..35eae1e00a 100644
--- a/ld/testsuite/ld-plugin/plugin-12.d
+++ b/ld/testsuite/ld-plugin/plugin-12.d
@@ -1,6 +1,6 @@
 #...
-.*: symbol `func' definition: 0, visibility: 0, resolution: 2
-.*: symbol `func1' definition: 0, visibility: 1, resolution: 3
-.*: symbol `func2' definition: 0, visibility: 2, resolution: 3
-.*: symbol `func3' definition: 0, visibility: 3, resolution: 3
+.*: symbol `func' definition: DEF, visibility: DEFAULT, resolution: PREVAILING_DEF
+.*: symbol `func1' definition: DEF, visibility: PROTECTED, resolution: PREVAILING_DEF_IRONLY
+.*: symbol `func2' definition: DEF, visibility: INTERNAL, resolution: PREVAILING_DEF_IRONLY
+.*: symbol `func3' definition: DEF, visibility: HIDDEN, resolution: PREVAILING_DEF_IRONLY
 #pass
-- 
2.21.0
Nick Clifton March 14, 2019, 10:42 a.m. | #4
Hi Martin,

> Good point, I reworked that.


Nice but ...

+/* Return LTO visibility string name that corresponds to INDEX enum value.  */
+static const char *
+get_lto_visibility (unsigned int index)

[...]

+  char *buf = xmalloc (64);
+  sprintf (buf, _("unknown LTO visibility value %x"), index);
+  return buf;

This is a memory leak.  (Trivial I know, but someone somewhere
will complain).  Plus it is a theoretical buffer overrun if the
translated version of the "unknown LTO visibility" string runs
to more than ~64 bytes.

To be safe, I would suggest that you use the concat() function
from libiberty, rather than xmalloc, and you either provide a
return flag indicating that a free is needed, or else just
allocate space for all of the returned strings.  If you think
that this is overkill (and to be honest I do) then just printing
into a large static buffer would be fine.

Cheers
  Nick

PS.  Of course these remarks apply to the other two new functions
as well.
Martin Liška March 15, 2019, 8:51 a.m. | #5
On 3/14/19 11:42 AM, Nick Clifton wrote:
> Hi Martin,

> 

>> Good point, I reworked that.

> 

> Nice but ...

> 

> +/* Return LTO visibility string name that corresponds to INDEX enum value.  */

> +static const char *

> +get_lto_visibility (unsigned int index)

> 

> [...]

> 

> +  char *buf = xmalloc (64);

> +  sprintf (buf, _("unknown LTO visibility value %x"), index);

> +  return buf;

> 

> This is a memory leak.  (Trivial I know, but someone somewhere

> will complain).  Plus it is a theoretical buffer overrun if the

> translated version of the "unknown LTO visibility" string runs

> to more than ~64 bytes.

> 

> To be safe, I would suggest that you use the concat() function

> from libiberty, rather than xmalloc, and you either provide a

> return flag indicating that a free is needed, or else just

> allocate space for all of the returned strings.  If you think

> that this is overkill (and to be honest I do) then just printing

> into a large static buffer would be fine.


Hi.

I like the static buffer for printing. Note that 64 should be fine
as len(str(1 << 64)) is about 20 characters.

Hope it's fine now?
Thanks,
Martin

> 

> Cheers

>   Nick

> 

> PS.  Of course these remarks apply to the other two new functions

> as well.

> 

> 

> 

>  

>
From 80815edc1f3d0bc229cba5d33395ad1832c94b29 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>

Date: Tue, 26 Feb 2019 15:19:12 +0100
Subject: [PATCH] Provide string description of definition, visibility and
 resolution in LTO plug-in.

ld/ChangeLog:

2019-02-26  Martin Liska  <mliska@suse.cz>

	* plugin.c (get_symbols): Add lto_kind_str, lto_resolution_str,
	lto_visibility_str and use then to inform about plugin-symbols.
	* testsuite/ld-plugin/plugin-12.d: Adjust expected pattern.
---
 ld/plugin.c                        | 73 +++++++++++++++++++++++++++++-
 ld/testsuite/ld-plugin/plugin-12.d |  8 ++--
 2 files changed, 75 insertions(+), 6 deletions(-)

diff --git a/ld/plugin.c b/ld/plugin.c
index ea1a7f7064..0e15654dcf 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -659,6 +659,73 @@ is_visible_from_outside (struct ld_plugin_symbol *lsym,
   return FALSE;
 }
 
+/* Return LTO kind string name that corresponds to INDEX enum value.  */
+static const char *
+get_lto_kind (unsigned int index)
+{
+  static char buffer[64];
+  const char *lto_kind_str[5] =
+  {
+    "DEF",
+    "WEAKDEF",
+    "UNDEF",
+    "WEAKUNDEF",
+    "COMMON"
+  };
+
+  if (index < ARRAY_SIZE (lto_kind_str))
+    return lto_kind_str [index];
+
+  sprintf (buffer, _("unknown LTO kind value %x"), index);
+  return buffer;
+}
+
+/* Return LTO resolution string name that corresponds to INDEX enum value.  */
+static const char *
+get_lto_resolution (unsigned int index)
+{
+  static char buffer[64];
+  static const char *lto_resolution_str[10] =
+  {
+    "UNKNOWN",
+    "UNDEF",
+    "PREVAILING_DEF",
+    "PREVAILING_DEF_IRONLY",
+    "PREEMPTED_REG",
+    "PREEMPTED_IR",
+    "RESOLVED_IR",
+    "RESOLVED_EXEC",
+    "RESOLVED_DYN",
+    "PREVAILING_DEF_IRONLY_EXP",
+  };
+
+  if (index < ARRAY_SIZE (lto_resolution_str))
+    return lto_resolution_str [index];
+
+  sprintf (buffer, _("unknown LTO resolution value %x"), index);
+  return buffer;
+}
+
+/* Return LTO visibility string name that corresponds to INDEX enum value.  */
+static const char *
+get_lto_visibility (unsigned int index)
+{
+  static char buffer[64];
+  const char *lto_visibility_str[4] =
+  {
+    "DEFAULT",
+    "PROTECTED",
+    "INTERNAL",
+    "HIDDEN"
+  };
+
+  if (index < ARRAY_SIZE (lto_visibility_str))
+    return lto_visibility_str [index];
+
+  sprintf (buffer, _("unknown LTO visibility value %x"), index);
+  return buffer;
+}
+
 /* Get the symbol resolution info for a plugin-claimed input file.  */
 static enum ld_plugin_status
 get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
@@ -777,9 +844,11 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
       syms[n].resolution = res;
       if (report_plugin_symbols)
 	einfo (_("%P: %pB: symbol `%s' "
-		 "definition: %d, visibility: %d, resolution: %d\n"),
+		 "definition: %s, visibility: %s, resolution: %s\n"),
 	       abfd, syms[n].name,
-	       syms[n].def, syms[n].visibility, res);
+	       get_lto_kind (syms[n].def),
+	       get_lto_visibility (syms[n].visibility),
+	       get_lto_resolution (res));
     }
   return LDPS_OK;
 }
diff --git a/ld/testsuite/ld-plugin/plugin-12.d b/ld/testsuite/ld-plugin/plugin-12.d
index 10d772553d..35eae1e00a 100644
--- a/ld/testsuite/ld-plugin/plugin-12.d
+++ b/ld/testsuite/ld-plugin/plugin-12.d
@@ -1,6 +1,6 @@
 #...
-.*: symbol `func' definition: 0, visibility: 0, resolution: 2
-.*: symbol `func1' definition: 0, visibility: 1, resolution: 3
-.*: symbol `func2' definition: 0, visibility: 2, resolution: 3
-.*: symbol `func3' definition: 0, visibility: 3, resolution: 3
+.*: symbol `func' definition: DEF, visibility: DEFAULT, resolution: PREVAILING_DEF
+.*: symbol `func1' definition: DEF, visibility: PROTECTED, resolution: PREVAILING_DEF_IRONLY
+.*: symbol `func2' definition: DEF, visibility: INTERNAL, resolution: PREVAILING_DEF_IRONLY
+.*: symbol `func3' definition: DEF, visibility: HIDDEN, resolution: PREVAILING_DEF_IRONLY
 #pass
-- 
2.21.0
Nick Clifton March 15, 2019, 10:03 a.m. | #6
Hi Martin,

> Hope it's fine now?


It is - thanks for persisting with this.

Patch approved - please apply.

Cheers
  Nick
Martin Liška March 19, 2019, 8:10 a.m. | #7
On 3/15/19 11:03 AM, Nick Clifton wrote:
> Hi Martin,

> 

>> Hope it's fine now?

> 

> It is - thanks for persisting with this.

> 

> Patch approved - please apply.

> 

> Cheers

>   Nick

> 


Thank you Nick.

May I please ask you for granting me a write access to
the official GIT repo?

Thanks,
Martin

Patch

diff --git a/ld/plugin.c b/ld/plugin.c
index ea1a7f7064..e86d22d432 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -165,6 +165,37 @@  static const enum ld_plugin_tag tv_header_tags[] =
   LDPT_SET_EXTRA_LIBRARY_PATH
 };
 
+static const char *lto_kind_str[5] =
+{
+  "DEF",
+  "WEAKDEF",
+  "UNDEF",
+  "WEAKUNDEF",
+  "COMMON"
+};
+
+static const char *lto_resolution_str[10] =
+{
+  "UNKNOWN",
+  "UNDEF",
+  "PREVAILING_DEF",
+  "PREVAILING_DEF_IRONLY",
+  "PREEMPTED_REG",
+  "PREEMPTED_IR",
+  "RESOLVED_IR",
+  "RESOLVED_EXEC",
+  "RESOLVED_DYN",
+  "PREVAILING_DEF_IRONLY_EXP",
+};
+
+const char *lto_visibility_str[4] =
+{
+  "DEFAULT",
+  "PROTECTED",
+  "INTERNAL",
+  "HIDDEN"
+};
+
 /* How many entries in the constant leading part of the tv array.  */
 static const size_t tv_header_size = ARRAY_SIZE (tv_header_tags);
 
@@ -777,9 +808,11 @@  get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
       syms[n].resolution = res;
       if (report_plugin_symbols)
 	einfo (_("%P: %pB: symbol `%s' "
-		 "definition: %d, visibility: %d, resolution: %d\n"),
+		 "definition: %s, visibility: %s, resolution: %s\n"),
 	       abfd, syms[n].name,
-	       syms[n].def, syms[n].visibility, res);
+	       lto_kind_str[syms[n].def],
+	       lto_visibility_str[syms[n].visibility],
+	       lto_resolution_str[res]);
     }
   return LDPS_OK;
 }
diff --git a/ld/testsuite/ld-plugin/plugin-12.d b/ld/testsuite/ld-plugin/plugin-12.d
index 10d772553d..35eae1e00a 100644
--- a/ld/testsuite/ld-plugin/plugin-12.d
+++ b/ld/testsuite/ld-plugin/plugin-12.d
@@ -1,6 +1,6 @@ 
 #...
-.*: symbol `func' definition: 0, visibility: 0, resolution: 2
-.*: symbol `func1' definition: 0, visibility: 1, resolution: 3
-.*: symbol `func2' definition: 0, visibility: 2, resolution: 3
-.*: symbol `func3' definition: 0, visibility: 3, resolution: 3
+.*: symbol `func' definition: DEF, visibility: DEFAULT, resolution: PREVAILING_DEF
+.*: symbol `func1' definition: DEF, visibility: PROTECTED, resolution: PREVAILING_DEF_IRONLY
+.*: symbol `func2' definition: DEF, visibility: INTERNAL, resolution: PREVAILING_DEF_IRONLY
+.*: symbol `func3' definition: DEF, visibility: HIDDEN, resolution: PREVAILING_DEF_IRONLY
 #pass