RFC: printing pointers to global (data) variable on Windows...

RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

Hello,

I have a piece of code that defines a struct with one of the fields
being a "char *". The field gets set to a global variable:

    char my_name[] = "MyName";

    k = {my_name, [...]};

When trying to print key.system_name on Windows, we get:

    (gdb) p k.system_name
    $2 = 0x403000 "MyName"

We're missing the reference to the global "my_name". On GNU/Linux,
we get:

    (gdb) p k.system_name
    $3 = 0x6008e8 <my_name> "MyName"

I couldn't understand why this wasn't working, since GDB does not seem
to have any problems determining what symbol is at 0x403000:

    (gdb) info symbol 0x403000
    my_name in section .data of c:\[...]\lb.exe

Debugging GDB, I found the following block of code in build_address_symbolic:

    if (msymbol != NULL
        && MSYMBOL_SIZE (msymbol) == 0
        && MSYMBOL_TYPE (msymbol) != mst_text
        && MSYMBOL_TYPE (msymbol) != mst_text_gnu_ifunc
        && MSYMBOL_TYPE (msymbol) != mst_file_text)
      msymbol = NULL;

Basically, build_address_symbolic searches for symbol names two
ways: through the minimal symbols, and through the symbol table,
but only function symbols:

    msymbol = lookup_minimal_symbol_by_pc_section (addr, section);
    symbol = find_pc_sect_function (addr, section);

I looked at the COFF/PE documentation, and I do not think that there
is a way to provide the size of each symbol in the symbol table.
So the block of code above discards our minimal symbol entry due to
the size (always) being zero. And since our global is not a function,
we do not find anything from the debugging info either.

Does anyone know why we have the block discarding zero-sized non-text
symbols? I ran the testsuite on x86_64-linux, and got no regression.

My second question is regarding the fact that we looking symbols for
functions only. This probably made sense if the function was used for
text addresses (like disass), but does it now?  Or perhaps it's the
GNU/Linux output that should be fixed, and only text symbols should
be printed when printing addresses (somehow, I do not think that this
would be right).

At the moment, I am thinking that the proper fix would probably be
two ways: ditch the block discarding zero-sized non-text symbols
(this breaks with symbol tables whose format do not allow for symbol
sizes), and also search all global symbols, rather than just functions.

Thoughts?

Thank you,
-- 
Joel

PS: I do not think that the problem is specific to the fact that this
    is a field inside a struct. I am pretty sure that the same would
    happen with a simple "char *" variable. But that's what I used for
    my investigation.

Re: RFC: printing pointers to global (data) variable on Windows...

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>     if (msymbol != NULL
Joel>         && MSYMBOL_SIZE (msymbol) == 0
Joel>         && MSYMBOL_TYPE (msymbol) != mst_text
Joel>         && MSYMBOL_TYPE (msymbol) != mst_text_gnu_ifunc
Joel>         && MSYMBOL_TYPE (msymbol) != mst_file_text)
Joel>       msymbol = NULL;

I think the reason for this is here:

    http://sourceware.org/ml/gdb-patches/2012-04/msg00175.html

text symbols are allowed since without them (according to a later
message) asm-source.exp would regress.

Joel> I looked at the COFF/PE documentation, and I do not think that there
Joel> is a way to provide the size of each symbol in the symbol table.
Joel> So the block of code above discards our minimal symbol entry due to
Joel> the size (always) being zero. And since our global is not a function,
Joel> we do not find anything from the debugging info either.

Joel> Does anyone know why we have the block discarding zero-sized non-text
Joel> symbols? I ran the testsuite on x86_64-linux, and got no regression.

I wonder what happens if you set the symbol sizes to 1.

Ok, horrible idea.  Perhaps some flag bit on the minsym instead?
Or on the objfile?

Joel> My second question is regarding the fact that we looking symbols for
Joel> functions only. This probably made sense if the function was used for
Joel> text addresses (like disass), but does it now?  Or perhaps it's the
Joel> GNU/Linux output that should be fixed, and only text symbols should
Joel> be printed when printing addresses (somehow, I do not think that this
Joel> would be right).

IIRC the full symbol tables only record address information for text
symbols, not for data symbols.  If so, one cannot do this lookup.

That's what I remember from when I wrote this change.  It would be nice
to be wrong since the current approach means we can't always print a
sensible answer for users.

Tom

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

> I think the reason for this is here:
> 
>     http://sourceware.org/ml/gdb-patches/2012-04/msg00175.html

Thanks for the pointer!

> Joel> My second question is regarding the fact that we looking symbols for
> Joel> functions only. This probably made sense if the function was used for
> Joel> text addresses (like disass), but does it now?  Or perhaps it's the
> Joel> GNU/Linux output that should be fixed, and only text symbols should
> Joel> be printed when printing addresses (somehow, I do not think that this
> Joel> would be right).
> 
> IIRC the full symbol tables only record address information for text
> symbols, not for data symbols.  If so, one cannot do this lookup.

I do not understand this part, though. Searches by name should return
global variables too, no?

> Ok, horrible idea.  Perhaps some flag bit on the minsym instead?
> Or on the objfile?

You mean, setting a flag that allows us to know that sizes are not
available, and thus avoid the heuristics/filtering? I wish we could
have consistent behavior instead, but that might not be achievable.
I thought about setting the size to -1, but that would open its own
can of worms, I imagine, and that's not doable either, because the
symbol size is an unsigned value (as it should be, if you ask me).
Looking at the definition of struct minimal_symbol, it looks like
we'd have some room for an extra flag. But I am not sure I really
want to go that way. The filtering wouldn't be a problem if we were
searching for all global symbols, not just functions.

-- 
Joel

Re: RFC: printing pointers to global (data) variable on Windows...

[Tom Tromey]

Tom> IIRC the full symbol tables only record address information for text
Tom> symbols, not for data symbols.  If so, one cannot do this lookup.

Joel> I do not understand this part, though. Searches by name should return
Joel> global variables too, no?

build_address_symbolic is searching for the symbol by PC:

  symbol = find_pc_sect_function (addr, section);

I think it would be reasonable, and maybe useful, if gdb could do
by-address searches for data symbols.  That was a bigger change than I
wanted to make for the "set print symbol" series though.

Tom> Ok, horrible idea.  Perhaps some flag bit on the minsym instead?
Tom> Or on the objfile?

Joel> You mean, setting a flag that allows us to know that sizes are not
Joel> available, and thus avoid the heuristics/filtering?

Yeah.

Joel> Looking at the definition of struct minimal_symbol, it looks like
Joel> we'd have some room for an extra flag.

Yes.  pahole is super for this kind of thing... it reports a pretty big
hole after target_flag_2.

Joel> But I am not sure I really
Joel> want to go that way. The filtering wouldn't be a problem if we were
Joel> searching for all global symbols, not just functions.

That would be best, but I'm out of ideas.
I think the existing check is really what we want for ELF.

Maybe for ELF we could find some way to mark symbols we don't want.
But that's just the flag idea but the sense reversed.

Tom

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

> I think it would be reasonable, and maybe useful, if gdb could do
> by-address searches for data symbols.  That was a bigger change than I
> wanted to make for the "set print symbol" series though.

As mentioned on IRC, I just realize, now, the kind of project this is...
So, I went with the other solution, which is to mark the minimal symbols
from COFF/PE as size-less, and avoid the filtering in that case.
Attached is a patch that does that.

The patch was tested on x86-windows (using AdaCore's testsuite),
as well as on x86_64-linux.  But I am not proposing it's inclusion,
for several reasons:

  1. I strongly dislike this patch. It feels like an intrusive change
     to work around a heuristic that seems very fragile. I'd rather
     get rid of the heuristic and accept the false matches.

     I am wondering what would happen if we changed the code such that:
     If we find a function in the debugging info that matches our
     address, then use that instead of trying to see if the minimal
     symbol might be closer. When would that happen, anyway?

  2. Applying the patch fixes the problem in the sense that we now
     get the name of the symbol next to the address.  But on the
     other hand, it causes us to pick up some other (unrelated)
     symbols which were previously ignored by the heuristic. This
     is unexpected.

After working on and off on this, I do not feel very confident that
I will be able to tune the filtering to something that would satisfy
me.  So, rather than trying to do our best, I thought we might try
to do the simplest (removing the filtering of zero-sized data symbols).

Another option is for the FSF version of GDB to remain as it is,
with its bias towards GNU/Linux, while we will change AdaCore's
version to turn "set print symbol" to "off" by default...

Thoughts?
-- 
Joel

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

> As mentioned on IRC, I just realize, now, the kind of project this is...
> So, I went with the other solution, which is to mark the minimal symbols
> from COFF/PE as size-less, and avoid the filtering in that case.
> Attached is a patch that does that.

Really attached, this time. Thanks, Tom.

-- 
Joel

[-- Attachment #2: 0001-WIP-Minimal-symbols-with-no-size-info.patch --]
[-- Type: text/x-diff, Size: 5923 bytes --]

From 9ddc3d640a76b98814d6599327406f25535fe3bb Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Sat, 18 Aug 2012 00:32:08 +0200
Subject: [PATCH] WIP: Minimal symbols with no size info.

---
 gdb/coff-pe-read.c |    6 ++++--
 gdb/coffread.c     |    5 ++++-
 gdb/minsyms.c      |    7 ++++---
 gdb/minsyms.h      |   19 ++++++++++++-------
 gdb/printcmd.c     |    1 +
 gdb/symtab.h       |    4 ++++
 6 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
index 66c7c82..070c8f7 100644
--- a/gdb/coff-pe-read.c
+++ b/gdb/coff-pe-read.c
@@ -108,6 +108,7 @@ add_pe_exported_sym (char *sym_name,
 
   char *qualified_name = 0;
   int dll_name_len = strlen (dll_name);
+  struct minimal_symbol *msym;
 
   /* Generate a (hopefully unique) qualified name using the first part
      of the dll name, e.g. KERNEL32!AddAtomA.  This matches the style
@@ -125,8 +126,9 @@ add_pe_exported_sym (char *sym_name,
   xfree (qualified_name);
 
   /* Enter the plain name as well, which might not be unique.  */
-  prim_record_minimal_symbol (sym_name, vma, 
-			      section_data->ms_type, objfile);
+  msym = prim_record_minimal_symbol (sym_name, vma,
+				     section_data->ms_type, objfile);
+  MSYMBOL_HAS_SIZE (msym) = 0;
 }
 
 /* Truncate a dll_name at the first dot character.  */
diff --git a/gdb/coffread.c b/gdb/coffread.c
index 0c7e6d9..ab8bd36 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -422,15 +422,18 @@ record_minimal_symbol (struct coff_symbol *cs, CORE_ADDR address,
 		       struct objfile *objfile)
 {
   struct bfd_section *bfd_section;
+  struct minimal_symbol *msym;
 
   /* We don't want TDESC entry points in the minimal symbol table.  */
   if (cs->c_name[0] == '@')
     return NULL;
 
   bfd_section = cs_to_bfd_section (cs, objfile);
-  return prim_record_minimal_symbol_and_info (cs->c_name, address,
+  msym = prim_record_minimal_symbol_and_info (cs->c_name, address,
 					      type, section,
 					      bfd_section, objfile);
+  MSYMBOL_HAS_SIZE (msym) = 0;
+  return msym;
 }
 \f
 /* coff_symfile_init ()
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 1070fff..02b64ea 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -842,7 +842,7 @@ init_minimal_symbol_collection (void)
 
 /* See minsyms.h.  */
 
-void
+struct minimal_symbol *
 prim_record_minimal_symbol (const char *name, CORE_ADDR address,
 			    enum minimal_symbol_type ms_type,
 			    struct objfile *objfile)
@@ -869,8 +869,8 @@ prim_record_minimal_symbol (const char *name, CORE_ADDR address,
       section = -1;
     }
 
-  prim_record_minimal_symbol_and_info (name, address, ms_type,
-				       section, NULL, objfile);
+  return prim_record_minimal_symbol_and_info (name, address, ms_type,
+					      section, NULL, objfile);
 }
 
 /* See minsyms.h.  */
@@ -938,6 +938,7 @@ prim_record_minimal_symbol_full (const char *name, int name_len, int copy_name,
   MSYMBOL_TARGET_FLAG_1 (msymbol) = 0;
   MSYMBOL_TARGET_FLAG_2 (msymbol) = 0;
   MSYMBOL_SIZE (msymbol) = 0;
+  MSYMBOL_HAS_SIZE (msymbol) = 1; /* See this function's documentation.  */
 
   /* The hash pointers must be cleared! If they're not,
      add_minsym_to_hash_table will NOT add this msymbol to the hash table.  */
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 8f0472f..76d7082 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -60,7 +60,14 @@ struct cleanup *make_cleanup_discard_minimal_symbols (void);
    though, to stash the pointer anywhere; as minimal symbols may be
    moved after creation.  The memory for the returned minimal symbol
    is still owned by the minsyms.c code, and should not be freed.
-   
+
+   By default, it is assumed that the symbol size can be determined,
+   which means that the "has_size" flag of the returned minimal symbol
+   is set to 1.  But some symbol table formats such as the one used
+   in COFF/PE, for instance, do not include that piece of information.
+   In that case, the "has_size" flag of the returned symbol should be
+   unset.
+
    Arguments are:
 
    NAME - the symbol's name
@@ -89,13 +96,11 @@ struct minimal_symbol *prim_record_minimal_symbol_full
    - uses strlen to compute NAME_LEN,
    - passes COPY_NAME = 0,
    - passes SECTION = 0,
-   - and passes BFD_SECTION = NULL.
-   
-   This variant does not return the new symbol.  */
+   - and passes BFD_SECTION = NULL.  */
 
-void prim_record_minimal_symbol (const char *, CORE_ADDR,
-				 enum minimal_symbol_type,
-				 struct objfile *);
+struct minimal_symbol *prim_record_minimal_symbol (const char *, CORE_ADDR,
+						   enum minimal_symbol_type,
+						   struct objfile *);
 
 /* Like prim_record_minimal_symbol_full, but:
    - uses strlen to compute NAME_LEN,
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index d5b5b63..9e8cd65 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -680,6 +680,7 @@ build_address_symbolic (struct gdbarch *gdbarch,
     }
 
   if (msymbol != NULL
+      && MSYMBOL_HAS_SIZE (msymbol)
       && MSYMBOL_SIZE (msymbol) == 0
       && MSYMBOL_TYPE (msymbol) != mst_text
       && MSYMBOL_TYPE (msymbol) != mst_text_gnu_ifunc
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 76120a3..f45f498 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -348,6 +348,9 @@ struct minimal_symbol
   unsigned int target_flag_1 : 1;
   unsigned int target_flag_2 : 1;
 
+  /* Should be zero if the size of the minimal symbol is not available.  */
+  unsigned int has_size : 1;
+
   /* Minimal symbols with the same hash key are kept on a linked
      list.  This is the link.  */
 
@@ -362,6 +365,7 @@ struct minimal_symbol
 #define MSYMBOL_TARGET_FLAG_1(msymbol)  (msymbol)->target_flag_1
 #define MSYMBOL_TARGET_FLAG_2(msymbol)  (msymbol)->target_flag_2
 #define MSYMBOL_SIZE(msymbol)		(msymbol)->size
+#define MSYMBOL_HAS_SIZE(msymbol)	(msymbol)->has_size
 #define MSYMBOL_TYPE(msymbol)		(msymbol)->type
 
 #include "minsyms.h"
-- 
1.7.1

Re: RFC: printing pointers to global (data) variable on Windows...

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>   1. I strongly dislike this patch. It feels like an intrusive change
Joel>      to work around a heuristic that seems very fragile. I'd rather
Joel>      get rid of the heuristic and accept the false matches.

I accept that, but there's another way of looking at it.

It is just a fact that some minimal symbol readers can supply size
information, while others cannot.  Right now, this difference is
obscured in the gdb internals -- if you have a minimal symbol you can't
determine whether the size is "zero but valid" or "invalid".

So, supplying the flag makes gdb better model reality.  In this sense it
is an improvement.  Now, whether it is a *useful* improvement... :)

Joel>      I am wondering what would happen if we changed the code such that:
Joel>      If we find a function in the debugging info that matches our
Joel>      address, then use that instead of trying to see if the minimal
Joel>      symbol might be closer. When would that happen, anyway?

Yeah, I don't know.

Joel> After working on and off on this, I do not feel very confident that
Joel> I will be able to tune the filtering to something that would satisfy
Joel> me.  So, rather than trying to do our best, I thought we might try
Joel> to do the simplest (removing the filtering of zero-sized data symbols).

It seems to me that the filter is correct: if you are on a platform
where minimal symbol sizes are valid, and you see a symbol of size zero,
it does not make sense to print such a symbol.  It is, I think, a symbol
that doesn't name a whole object, but rather just some marker in the
assembly.

The code for the filter is actually somewhat goofier, allowing text
symbols through.  That just corresponds to gdb's historical behavior; I
am not sure whether or not it is reasonable behavior, but it didn't make
sense to change it in the context of that patch.  At least, the intent
is to have it provide the historical behavior; in the past I think
build_address_symbolic was only called for functions.

Joel> Another option is for the FSF version of GDB to remain as it is,
Joel> with its bias towards GNU/Linux, while we will change AdaCore's
Joel> version to turn "set print symbol" to "off" by default...

If you disable "set print symbol", how would that differ from leaving
the filter in place?

Tom

Re: RFC: printing pointers to global (data) variable on Windows...

[Tom Tromey]

Joel> +   By default, it is assumed that the symbol size can be determined,
Joel> +   which means that the "has_size" flag of the returned minimal symbol
Joel> +   is set to 1.

What about defaulting it to zero, applying the appended, and then fixing
up the fallout?

I could do the mechanics of this if you think it is ok; and if you plan
to go forward with this.

Tom

diff --git a/gdb/symtab.h b/gdb/symtab.h
index 5401b22..05bc706 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -361,7 +361,12 @@ struct minimal_symbol
 
 #define MSYMBOL_TARGET_FLAG_1(msymbol)  (msymbol)->target_flag_1
 #define MSYMBOL_TARGET_FLAG_2(msymbol)  (msymbol)->target_flag_2
-#define MSYMBOL_SIZE(msymbol)		(msymbol)->size
+#define MSYMBOL_SIZE(msymbol)		((msymbol)->size + 0)
+#define SET_MSYMBOL_SIZE(msymbol, sz)		\
+  do {						\
+    (msymbol)->size = sz;			\
+    (msymbol)->has_size = 1;			\
+  } while (0)
 #define MSYMBOL_TYPE(msymbol)		(msymbol)->type
 
 #include "minsyms.h"

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

> What about defaulting it to zero, applying the appended, and then fixing
> up the fallout?

I thought about that too, and got concerned about missing some places.
However, I very much like one thing about your patch, which is the fact
that it now forces the use of a "setter" to change the value of the
msymbol size, and that answers my main worry. And just because of the
"setter" alone, I'd go with this approach.

We could go with this appraoch independently of the has_size flag first,
and then add the flag. The one tiny concern is for readers that did not
set the size of zero-sized symbols because it knew that the default
was already zero.

> I could do the mechanics of this if you think it is ok; and if you plan
> to go forward with this.

Thanks for the kind offer - I brought this up, I should really be
doing the work...

-- 
Joel

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

> So, supplying the flag makes gdb better model reality.  In this sense it
> is an improvement.  Now, whether it is a *useful* improvement... :)

This is true.

> It seems to me that the filter is correct: if you are on a platform
> where minimal symbol sizes are valid, and you see a symbol of size zero,
> it does not make sense to print such a symbol.

OK, that makes sense. We might even want to go a little further and
exclude symbols if our address is past the end of the symbol. For
instance, if we have a symbol "bla" at 0x1000, and its size is 4 bytes,
and we're calling our routine with 0x1008, I am not sure it makes
much sense saying 0x1008 is <bla+8>. But maybe it does?


> Joel> Another option is for the FSF version of GDB to remain as it is,
> Joel> with its bias towards GNU/Linux, while we will change AdaCore's
> Joel> version to turn "set print symbol" to "off" by default...
> 
> If you disable "set print symbol", how would that differ from leaving
> the filter in place?

I was just sweeping the problem under the rug... Not making it the
default is my way of telling the users that the feature is not
completely prime-time; and so the minor quirks you can get on some
of the systems are to be expected. It would also save me a ton of
time not having to tweak the expected output in the testsuite, but
that's a minor detail.

I haven't decided about what to do with the default, but regardless,
I tend to think that you're right about flagging size-less symbols.
It will effectively disable the filter on COFF platforms, allowing
some symbols that would otherwise be filtered, but I guess that's
better than ignoring too many symbols...

I'll try working on that today.

Thanks, Tom.
-- 
Joel

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

Hello,

Here is a new version of the patch implementing Tom's idea of
flagging size-less minimal symbols... On platforms where he size
isn't set, i's a bit of a give-and-take, fixing the problem reported,
at the cost of finding some symbols that we were previously fitering
out (by luck or accident). I think it's for the best.

gdb/ChangeLog:

        * symtab.h (struct minimal_symbol) [has_size]: New field.
        (MSYMBOL_SIZE): Adjust to forbid macro from being used as lvalue.
        (SET_MSYMBOL_SIZE, MSYMBOL_HAS_SIZE): New macros.
        * printcmd.c (build_address_symbolic): Only filter out zero-sized
        minimal symbols if the symbol's size is actually known.
        * minsyms.c (prim_record_minimal_symbol_full): Adjust setting
        of msymbol's size field.  Add comment.
        (install_minimal_symbols): Use SET_MSYMBOL_SIZE to set the
        minimal symbol size.
        * elfread.c (elf_symtab_read, elf_rel_plt_read): Use
        SET_MSYMBOL_SIZE to set the minimal symbol size.

Tested on all of AdaCore's supported platforms via our testsuite.
Also tested on x86_64-linux using the official testsuite.

OK to commit?

Thanks,
-- 
Joel

[-- Attachment #2: 0001-Name-of-symbol-missing-when-printing-global-variable.patch --]
[-- Type: text/x-diff, Size: 5295 bytes --]

From b70bf2abc01142b7d93c6fd834d9926bdc9eea9c Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 22 Aug 2012 20:29:13 +0200
Subject: [PATCH] Name of symbol missing when printing global variable's address

The build_address_symbolic funnction filters out data symbols if
their size is set to zero.  But the problem is that the COFF symbol
table (for instance) does not provide any size information, leaving
the size to its default value of zero, thus always triggering
the filter.

This shows up when trying to print the address of a global variable
when debugging a Windows executable, for instance.

gdb/ChangeLog:

        * symtab.h (struct minimal_symbol) [has_size]: New field.
        (MSYMBOL_SIZE): Adjust to forbid macro from being used as lvalue.
        (SET_MSYMBOL_SIZE, MSYMBOL_HAS_SIZE): New macros.
        * printcmd.c (build_address_symbolic): Only filter out zero-sized
        minimal symbols if the symbol's size is actually known.
        * minsyms.c (prim_record_minimal_symbol_full): Adjust setting
        of msymbol's size field.  Add comment.
        (install_minimal_symbols): Use SET_MSYMBOL_SIZE to set the
        minimal symbol size.
        * elfread.c (elf_symtab_read, elf_rel_plt_read): Use
        SET_MSYMBOL_SIZE to set the minimal symbol size.
---
 gdb/elfread.c  |    6 +++---
 gdb/minsyms.c  |    6 ++++--
 gdb/printcmd.c |    1 +
 gdb/symtab.h   |   14 +++++++++++++-
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/gdb/symtab.h b/gdb/symtab.h
index 76120a3..041d8cf 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -348,6 +348,11 @@ struct minimal_symbol
   unsigned int target_flag_1 : 1;
   unsigned int target_flag_2 : 1;
 
+  /* Nonzero iff the size of the minimal symbol has been set.
+     Symbol size information can sometimes not be determined, because
+     the object file format may not carry that piece of information.  */
+  unsigned int has_size : 1;
+
   /* Minimal symbols with the same hash key are kept on a linked
      list.  This is the link.  */
 
@@ -361,7 +366,14 @@ struct minimal_symbol
 
 #define MSYMBOL_TARGET_FLAG_1(msymbol)  (msymbol)->target_flag_1
 #define MSYMBOL_TARGET_FLAG_2(msymbol)  (msymbol)->target_flag_2
-#define MSYMBOL_SIZE(msymbol)		(msymbol)->size
+#define MSYMBOL_SIZE(msymbol)		((msymbol)->size + 0)
+#define SET_MSYMBOL_SIZE(msymbol, sz)		\
+  do						\
+    {						\
+      (msymbol)->size = sz;			\
+      (msymbol)->has_size = 1;			\
+    } while (0)
+#define MSYMBOL_HAS_SIZE(msymbol)	((msymbol)->has_size + 0)
 #define MSYMBOL_TYPE(msymbol)		(msymbol)->type
 
 #include "minsyms.h"
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index d5b5b63..9e8cd65 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -680,6 +680,7 @@ build_address_symbolic (struct gdbarch *gdbarch,
     }
 
   if (msymbol != NULL
+      && MSYMBOL_HAS_SIZE (msymbol)
       && MSYMBOL_SIZE (msymbol) == 0
       && MSYMBOL_TYPE (msymbol) != mst_text
       && MSYMBOL_TYPE (msymbol) != mst_text_gnu_ifunc
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 1070fff..2bad947 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -937,7 +937,9 @@ prim_record_minimal_symbol_full (const char *name, int name_len, int copy_name,
   MSYMBOL_TYPE (msymbol) = ms_type;
   MSYMBOL_TARGET_FLAG_1 (msymbol) = 0;
   MSYMBOL_TARGET_FLAG_2 (msymbol) = 0;
-  MSYMBOL_SIZE (msymbol) = 0;
+  /* Do not use the SET_MSYMBOL_SIZE macro to initialize the size,
+     as it would also set the has_size flag.  */
+  msymbol->size = 0;
 
   /* The hash pointers must be cleared! If they're not,
      add_minsym_to_hash_table will NOT add this msymbol to the hash table.  */
@@ -1233,7 +1235,7 @@ install_minimal_symbols (struct objfile *objfile)
       SYMBOL_VALUE_ADDRESS (&msymbols[mcount]) = 0;
       MSYMBOL_TARGET_FLAG_1 (&msymbols[mcount]) = 0;
       MSYMBOL_TARGET_FLAG_2 (&msymbols[mcount]) = 0;
-      MSYMBOL_SIZE (&msymbols[mcount]) = 0;
+      SET_MSYMBOL_SIZE (&msymbols[mcount], 0);
       MSYMBOL_TYPE (&msymbols[mcount]) = mst_unknown;
       SYMBOL_SET_LANGUAGE (&msymbols[mcount], language_unknown);
 
diff --git a/gdb/elfread.c b/gdb/elfread.c
index f3967d7..516cbd0 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -570,7 +570,7 @@ elf_symtab_read (struct objfile *objfile, int type,
 		elf_sym = (elf_symbol_type *) sym->udata.p;
 
 	      if (elf_sym)
-		MSYMBOL_SIZE(msym) = elf_sym->internal_elf_sym.st_size;
+		SET_MSYMBOL_SIZE (msym, elf_sym->internal_elf_sym.st_size);
 
 	      msym->filename = filesymname;
 	      gdbarch_elf_make_msymbol_special (gdbarch, sym, msym);
@@ -594,7 +594,7 @@ elf_symtab_read (struct objfile *objfile, int type,
 						  sym->section, objfile);
 		  if (mtramp)
 		    {
-		      MSYMBOL_SIZE (mtramp) = MSYMBOL_SIZE (msym);
+		      SET_MSYMBOL_SIZE (mtramp, MSYMBOL_SIZE (msym));
 		      mtramp->created_by_gdb = 1;
 		      mtramp->filename = filesymname;
 		      gdbarch_elf_make_msymbol_special (gdbarch, sym, mtramp);
@@ -689,7 +689,7 @@ elf_rel_plt_read (struct objfile *objfile, asymbol **dyn_symbol_table)
                                     1, address, mst_slot_got_plt, got_plt,
 				    objfile);
       if (msym)
-	MSYMBOL_SIZE (msym) = ptr_size;
+	SET_MSYMBOL_SIZE (msym, ptr_size);
     }
 
   do_cleanups (back_to);
-- 
1.7.1

Re: RFC: printing pointers to global (data) variable on Windows...

[asmwarrior]

On 2012-9-5 22:44, Joel Brobecker wrote:
> Nonzero iff the size of the minimal symbol has been set.
           ^^^
Typo in comments.

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

> >Nonzero iff the size of the minimal symbol has been set.
>           ^^^
> Typo in comments.

That's actually not a typo: "iff" means "if and only if".

-- 
Joel

Re: RFC: printing pointers to global (data) variable on Windows...

[asmwarrior]

On 2012-9-6 9:44, Joel Brobecker wrote:
>>> Nonzero iff the size of the minimal symbol has been set.
>> >           ^^^
>> >Typo in comments.
> That's actually not a typo: "iff" means "if and only if".

Oh, I see. Thanks.

Re: RFC: printing pointers to global (data) variable on Windows...

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>         * symtab.h (struct minimal_symbol) [has_size]: New field.
Joel>         (MSYMBOL_SIZE): Adjust to forbid macro from being used as lvalue.
Joel>         (SET_MSYMBOL_SIZE, MSYMBOL_HAS_SIZE): New macros.
Joel>         * printcmd.c (build_address_symbolic): Only filter out zero-sized
Joel>         minimal symbols if the symbol's size is actually known.
Joel>         * minsyms.c (prim_record_minimal_symbol_full): Adjust setting
Joel>         of msymbol's size field.  Add comment.
Joel>         (install_minimal_symbols): Use SET_MSYMBOL_SIZE to set the
Joel>         minimal symbol size.
Joel>         * elfread.c (elf_symtab_read, elf_rel_plt_read): Use
Joel>         SET_MSYMBOL_SIZE to set the minimal symbol size.

Joel> OK to commit?

Joel> @@ -1233,7 +1235,7 @@ install_minimal_symbols (struct objfile *objfile)
Joel>        SYMBOL_VALUE_ADDRESS (&msymbols[mcount]) = 0;
Joel>        MSYMBOL_TARGET_FLAG_1 (&msymbols[mcount]) = 0;
Joel>        MSYMBOL_TARGET_FLAG_2 (&msymbols[mcount]) = 0;
Joel> -      MSYMBOL_SIZE (&msymbols[mcount]) = 0;
Joel> +      SET_MSYMBOL_SIZE (&msymbols[mcount], 0);

I tend to think this hunk should not use SET_MSYMBOL_SIZE.
Maybe it even ought to use memset instead of what it currently does.

Or, if SET_MSYMBOL_SIZE is correct here, a comment would be helpful.

The rest looked good to me.

Tom

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

> Joel> @@ -1233,7 +1235,7 @@ install_minimal_symbols (struct objfile *objfile)
> Joel>        SYMBOL_VALUE_ADDRESS (&msymbols[mcount]) = 0;
> Joel>        MSYMBOL_TARGET_FLAG_1 (&msymbols[mcount]) = 0;
> Joel>        MSYMBOL_TARGET_FLAG_2 (&msymbols[mcount]) = 0;
> Joel> -      MSYMBOL_SIZE (&msymbols[mcount]) = 0;
> Joel> +      SET_MSYMBOL_SIZE (&msymbols[mcount], 0);
> 
> I tend to think this hunk should not use SET_MSYMBOL_SIZE.
> Maybe it even ought to use memset instead of what it currently does.
> 
> Or, if SET_MSYMBOL_SIZE is correct here, a comment would be helpful.

Intuitively, I do not think that it really matters. But I prefer
the idea of memset-ting the whole object to zero, and it's actually
something I have been wanting to do anyways. So here are two patches.
The first one just adjusts install_minimal_symbols to use memset instead
of the various macros.

gdb/ChangeLog:

        * minsyms.c (install_minimal_symbols): Use memset to fill entire
        minimal_symbol struct object, rather than setting some of its
        fields one by one.

And the second is the initial patch, adjusted to the first one.

gdb/ChangeLog:

        * symtab.h (struct minimal_symbol) [has_size]: New field.
        (MSYMBOL_SIZE): Adjust to forbid macro from being used as lvalue.
        (SET_MSYMBOL_SIZE, MSYMBOL_HAS_SIZE): New macros.
        * printcmd.c (build_address_symbolic): Only filter out zero-sized
        minimal symbols if the symbol's size is actually known.
        * minsyms.c (prim_record_minimal_symbol_full): Adjust setting
        of msymbol's size field.  Add comment.
        * elfread.c (elf_symtab_read, elf_rel_plt_read): Use
        SET_MSYMBOL_SIZE to set the minimal symbol size.

Tested on x86_64-linux. OK to apply?

Thanks!
-- 
Joel

[-- Attachment #2: 0001-install_minimal_symbols-use-memset-instead-of-settin.patch --]
[-- Type: text/x-diff, Size: 1440 bytes --]

From 258f4f2dfdf021acb91bffd1f0ac95ebbaa7fbe6 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 10 Sep 2012 16:31:28 -0400
Subject: [PATCH 1/2] install_minimal_symbols: use memset instead of setting each field.

gdb/ChangeLog:

        * minsyms.c (install_minimal_symbols): Use memset to fill entire
        minimal_symbol struct object, rather than setting some of its
        fields one by one.
---
 gdb/minsyms.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 1070fff..b6df4ea 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1229,13 +1229,7 @@ install_minimal_symbols (struct objfile *objfile)
          symbol count does *not* include this null symbol, which is why it
          is indexed by mcount and not mcount-1.  */
 
-      SYMBOL_LINKAGE_NAME (&msymbols[mcount]) = NULL;
-      SYMBOL_VALUE_ADDRESS (&msymbols[mcount]) = 0;
-      MSYMBOL_TARGET_FLAG_1 (&msymbols[mcount]) = 0;
-      MSYMBOL_TARGET_FLAG_2 (&msymbols[mcount]) = 0;
-      MSYMBOL_SIZE (&msymbols[mcount]) = 0;
-      MSYMBOL_TYPE (&msymbols[mcount]) = mst_unknown;
-      SYMBOL_SET_LANGUAGE (&msymbols[mcount], language_unknown);
+      memset (&msymbols[mcount], 0, sizeof (struct minimal_symbol));
 
       /* Attach the minimal symbol table to the specified objfile.
          The strings themselves are also located in the objfile_obstack
-- 
1.7.0.4


[-- Attachment #3: 0002-Name-of-symbol-missing-when-printing-global-variable.patch --]
[-- Type: text/x-diff, Size: 4754 bytes --]

From 0da12186cb6f455a43f8937cb2b5dfcc39c2953c Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 22 Aug 2012 20:29:13 +0200
Subject: [PATCH 2/2] Name of symbol missing when printing global variable's address

The build_address_symbolic funnction filters out data symbols if
their size is set to zero.  But the problem is that the COFF symbol
table (for instance) does not provide any size information, leaving
the size to its default value of zero, thus always triggering
the filter.

This shows up when trying to print the address of a global variable
when debugging a Windows executable, for instance.

gdb/ChangeLog:

        * symtab.h (struct minimal_symbol) [has_size]: New field.
        (MSYMBOL_SIZE): Adjust to forbid macro from being used as lvalue.
        (SET_MSYMBOL_SIZE, MSYMBOL_HAS_SIZE): New macros.
        * printcmd.c (build_address_symbolic): Only filter out zero-sized
        minimal symbols if the symbol's size is actually known.
        * minsyms.c (prim_record_minimal_symbol_full): Adjust setting
        of msymbol's size field.  Add comment.
        * elfread.c (elf_symtab_read, elf_rel_plt_read): Use
        SET_MSYMBOL_SIZE to set the minimal symbol size.
---
 gdb/elfread.c  |    6 +++---
 gdb/minsyms.c  |    4 +++-
 gdb/printcmd.c |    1 +
 gdb/symtab.h   |   14 +++++++++++++-
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/gdb/elfread.c b/gdb/elfread.c
index f3967d7..516cbd0 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -570,7 +570,7 @@ elf_symtab_read (struct objfile *objfile, int type,
 		elf_sym = (elf_symbol_type *) sym->udata.p;
 
 	      if (elf_sym)
-		MSYMBOL_SIZE(msym) = elf_sym->internal_elf_sym.st_size;
+		SET_MSYMBOL_SIZE (msym, elf_sym->internal_elf_sym.st_size);
 
 	      msym->filename = filesymname;
 	      gdbarch_elf_make_msymbol_special (gdbarch, sym, msym);
@@ -594,7 +594,7 @@ elf_symtab_read (struct objfile *objfile, int type,
 						  sym->section, objfile);
 		  if (mtramp)
 		    {
-		      MSYMBOL_SIZE (mtramp) = MSYMBOL_SIZE (msym);
+		      SET_MSYMBOL_SIZE (mtramp, MSYMBOL_SIZE (msym));
 		      mtramp->created_by_gdb = 1;
 		      mtramp->filename = filesymname;
 		      gdbarch_elf_make_msymbol_special (gdbarch, sym, mtramp);
@@ -689,7 +689,7 @@ elf_rel_plt_read (struct objfile *objfile, asymbol **dyn_symbol_table)
                                     1, address, mst_slot_got_plt, got_plt,
 				    objfile);
       if (msym)
-	MSYMBOL_SIZE (msym) = ptr_size;
+	SET_MSYMBOL_SIZE (msym, ptr_size);
     }
 
   do_cleanups (back_to);
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index b6df4ea..a3a376c 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -937,7 +937,9 @@ prim_record_minimal_symbol_full (const char *name, int name_len, int copy_name,
   MSYMBOL_TYPE (msymbol) = ms_type;
   MSYMBOL_TARGET_FLAG_1 (msymbol) = 0;
   MSYMBOL_TARGET_FLAG_2 (msymbol) = 0;
-  MSYMBOL_SIZE (msymbol) = 0;
+  /* Do not use the SET_MSYMBOL_SIZE macro to initialize the size,
+     as it would also set the has_size flag.  */
+  msymbol->size = 0;
 
   /* The hash pointers must be cleared! If they're not,
      add_minsym_to_hash_table will NOT add this msymbol to the hash table.  */
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index d5b5b63..9e8cd65 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -680,6 +680,7 @@ build_address_symbolic (struct gdbarch *gdbarch,
     }
 
   if (msymbol != NULL
+      && MSYMBOL_HAS_SIZE (msymbol)
       && MSYMBOL_SIZE (msymbol) == 0
       && MSYMBOL_TYPE (msymbol) != mst_text
       && MSYMBOL_TYPE (msymbol) != mst_text_gnu_ifunc
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 76120a3..041d8cf 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -348,6 +348,11 @@ struct minimal_symbol
   unsigned int target_flag_1 : 1;
   unsigned int target_flag_2 : 1;
 
+  /* Nonzero iff the size of the minimal symbol has been set.
+     Symbol size information can sometimes not be determined, because
+     the object file format may not carry that piece of information.  */
+  unsigned int has_size : 1;
+
   /* Minimal symbols with the same hash key are kept on a linked
      list.  This is the link.  */
 
@@ -361,7 +366,14 @@ struct minimal_symbol
 
 #define MSYMBOL_TARGET_FLAG_1(msymbol)  (msymbol)->target_flag_1
 #define MSYMBOL_TARGET_FLAG_2(msymbol)  (msymbol)->target_flag_2
-#define MSYMBOL_SIZE(msymbol)		(msymbol)->size
+#define MSYMBOL_SIZE(msymbol)		((msymbol)->size + 0)
+#define SET_MSYMBOL_SIZE(msymbol, sz)		\
+  do						\
+    {						\
+      (msymbol)->size = sz;			\
+      (msymbol)->has_size = 1;			\
+    } while (0)
+#define MSYMBOL_HAS_SIZE(msymbol)	((msymbol)->has_size + 0)
 #define MSYMBOL_TYPE(msymbol)		(msymbol)->type
 
 #include "minsyms.h"
-- 
1.7.0.4

Re: RFC: printing pointers to global (data) variable on Windows...

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Intuitively, I do not think that it really matters. But I prefer
Joel> the idea of memset-ting the whole object to zero, and it's actually
Joel> something I have been wanting to do anyways. So here are two patches.
Joel> The first one just adjusts install_minimal_symbols to use memset instead
Joel> of the various macros.

Thanks, Joel.

Joel> Tested on x86_64-linux. OK to apply?

Looks good to me.

Tom

Re: RFC: printing pointers to global (data) variable on Windows...

[Joel Brobecker]

> Joel> Tested on x86_64-linux. OK to apply?
> 
> Looks good to me.

Thanks, Tom. Both patches now checked in :).

-- 
Joel