Revisit gdb/12528 for bare metal targets

Revisit gdb/12528 for bare metal targets

[Yao Qi]

I see the following fail on arm-none-eabi target,

(gdb) b 24^M
Breakpoint 1 at 0x4: file
../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
line 24.^M
(gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24

This test case is for PR gdb/12528, but I don't think this PR is fixed
for bare metal targets.

Paul asked for the advice for this PR
<https://sourceware.org/ml/gdb-patches/2011-03/msg00662.html> about how
to determine whether an address zero in debug info means the
corresponding code has been GC'ed.
Then, flag has_section_at_zero was chosen and the code is like:

	case DW_LNE_set_address:
	  address = read_address (abfd, line_ptr, cu, &bytes_read);

	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
	    {
	      /* This line table is for a function which has been
		 GCd by the linker.  Ignore it.  PR gdb/12528 */

On some bare metal targets, .text section is located at 0x0 so
has_section_at_zero is true.  That is why this test fails.  I am
looking for something else to check, for example, if ADDRESS is zero
and address zero isn't within any SEC_LOAD sections of CU
("break-on-linker-gcd-function.cc"), then the function should be GC'ed
by the linker.  I am not familiar with the stuff about CU and symbols,
so I'd like your advice on it.  TIA.

-- 
Yao (齐尧)

Re: Revisit gdb/12528 for bare metal targets

[Yao Qi]

On 07/30/2014 03:44 PM, Yao Qi wrote:
> I am
> looking for something else to check, for example, if ADDRESS is zero
> and address zero isn't within any SEC_LOAD sections of CU
> ("break-on-linker-gcd-function.cc"), then the function should be GC'ed
> by the linker.

I find I can check 'psymtab->textlow > 0'.  If psymtab->textlow is
greater than zero, and the address is zero, then the corresponding
function is GC'ed.  I'll post a patch soon.

-- 
Yao (齐尧)

[PATCH 1/2] Remove pst from dwarf_decode_lines_1

[Yao Qi]

Hi,
Parameter 'pst' of function dwarf_decode_lines_1 isn't used except
to compute decode_for_pst_p, which has been got in the caller
dwarf_decode_lines.  I wonder it would be good if we just pass
'decode_for_pst_p'.

gdb:

2014-08-06  Yao Qi  <yao@codesourcery.com>

	* dwarf2read.c (dwarf_decode_lines_1): Remove parameter 'pst'.
	Add parameter 'decode_for_pst_p'.  Callers update.
---
 gdb/dwarf2read.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 8f5d9d4..8011e4e 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17179,7 +17179,7 @@ noop_record_line (struct subfile *subfile, int line, CORE_ADDR pc)
 
 static void
 dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
-		      struct dwarf2_cu *cu, struct partial_symtab *pst)
+		      struct dwarf2_cu *cu, const int decode_for_pst_p)
 {
   const gdb_byte *line_ptr, *extended_end;
   const gdb_byte *line_end;
@@ -17189,7 +17189,6 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
   struct objfile *objfile = cu->objfile;
   bfd *abfd = objfile->obfd;
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
-  const int decode_for_pst_p = (pst != NULL);
   struct subfile *last_subfile = NULL;
   void (*p_record_line) (struct subfile *subfile, int line, CORE_ADDR pc)
     = record_line;
@@ -17507,7 +17506,7 @@ dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
   struct subfile *first_subfile = current_subfile;
 
   if (want_line_info)
-    dwarf_decode_lines_1 (lh, comp_dir, cu, pst);
+    dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p);
 
   if (decode_for_pst_p)
     {
-- 
1.9.0

[PATCH 2/2] Check function is GC'ed

[Yao Qi]

I see the following fail on arm-none-eabi target,

(gdb) b 24^M
Breakpoint 1 at 0x4: file
../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
line 24.^M
(gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24

Currently, we are using flag has_section_at_zero to determine whether
address zero in debug info means the corresponding code has been
GC'ed, like this:

	case DW_LNE_set_address:
	  address = read_address (abfd, line_ptr, cu, &bytes_read);

	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
	    {
	      /* This line table is for a function which has been
		 GCd by the linker.  Ignore it.  PR gdb/12528 */

However, this is incorrect on some bare metal targets, as .text
section is located at 0x0.  In this patch, we choose 'textlow' field
of partial symtabl.  This patch fixes the fail above.  It is
regression tested on x86_64-linux, arm-none-eabi,
arm-none-linux-gnueabi.  OK to apply?

gdb:

2014-08-06  Yao Qi  <yao@codesourcery.com>

	* dwarf2read.c (dwarf_decode_lines_1): Skip the line table if
	PST->textlow is greater than zero.
---
 gdb/dwarf2read.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 8011e4e..5e292f2 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17229,6 +17229,8 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
       /* Decode the table.  */
       while (!end_sequence)
 	{
+	  struct partial_symtab *pst = NULL;
+
 	  op_code = read_1_byte (abfd, line_ptr);
 	  line_ptr += 1;
 	  if (line_ptr > line_end)
@@ -17291,7 +17293,12 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 		case DW_LNE_set_address:
 		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 
-		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
+		  if (!decode_for_pst_p)
+		    pst = cu->per_cu->v.psymtab;
+
+		  if (address == 0
+		      && (!dwarf2_per_objfile->has_section_at_zero
+			  || (pst != NULL && pst->textlow > address)))
 		    {
 		      /* This line table is for a function which has been
 			 GCd by the linker.  Ignore it.  PR gdb/12528 */
-- 
1.9.0

Re: [PATCH 2/2] Check function is GC'ed

[Doug Evans]

Yao Qi writes:
 > I see the following fail on arm-none-eabi target,
 > 
 > (gdb) b 24^M
 > Breakpoint 1 at 0x4: file
 > ../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
 > line 24.^M
 > (gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24
 > 
 > Currently, we are using flag has_section_at_zero to determine whether
 > address zero in debug info means the corresponding code has been
 > GC'ed, like this:
 > 
 > 	case DW_LNE_set_address:
 > 	  address = read_address (abfd, line_ptr, cu, &bytes_read);
 > 
 > 	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > 	    {
 > 	      /* This line table is for a function which has been
 > 		 GCd by the linker.  Ignore it.  PR gdb/12528 */
 > 
 > However, this is incorrect on some bare metal targets, as .text
 > section is located at 0x0.  In this patch, we choose 'textlow' field
 > of partial symtabl.  This patch fixes the fail above.  It is
 > regression tested on x86_64-linux, arm-none-eabi,
 > arm-none-linux-gnueabi.  OK to apply?

Something in this explanation doesn't feel right.
If .text is at zero then has_section_at_zero should be true.
Perhaps the explanation just needs more elaboration,
but looking at break-on-linker-gcd-function.exp
the problem seems to be more that the test is invalid
when .text begins at 0x0.
If the testcase is invalid in this context (and
we can discuss ways in which to cope with that),
is there still a real bug here?

 > 
 > gdb:
 > 
 > 2014-08-06  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* dwarf2read.c (dwarf_decode_lines_1): Skip the line table if
 > 	PST->textlow is greater than zero.
 > ---
 >  gdb/dwarf2read.c | 9 ++++++++-
 >  1 file changed, 8 insertions(+), 1 deletion(-)
 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index 8011e4e..5e292f2 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -17229,6 +17229,8 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >        /* Decode the table.  */
 >        while (!end_sequence)
 >  	{
 > +	  struct partial_symtab *pst = NULL;
 > +
 >  	  op_code = read_1_byte (abfd, line_ptr);
 >  	  line_ptr += 1;
 >  	  if (line_ptr > line_end)
 > @@ -17291,7 +17293,12 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >  		case DW_LNE_set_address:
 >  		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 >  
 > -		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +		  if (!decode_for_pst_p)
 > +		    pst = cu->per_cu->v.psymtab;

I believe this is incorrect: .gdb_index may be in use and thus we're not using partial syms.

 > +
 > +		  if (address == 0
 > +		      && (!dwarf2_per_objfile->has_section_at_zero
 > +			  || (pst != NULL && pst->textlow > address)))
 >  		    {
 >  		      /* This line table is for a function which has been
 >  			 GCd by the linker.  Ignore it.  PR gdb/12528 */

Re: [PATCH 2/2] Check function is GC'ed

[Yao Qi]

On 08/15/2014 12:40 PM, Doug Evans wrote:
> Something in this explanation doesn't feel right.
> If .text is at zero then has_section_at_zero should be true.

Right.  Under this situation, if a function is GC'ed by linker,
the address is zero.  GDB thinks address zero is about a function's
address, rather than this function is GC'ed.

> Perhaps the explanation just needs more elaboration,

OK, I'll improve the explanation.

> but looking at break-on-linker-gcd-function.exp
> the problem seems to be more that the test is invalid
> when .text begins at 0x0.
> If the testcase is invalid in this context (and
> we can discuss ways in which to cope with that),
> is there still a real bug here?

AFAICS, the test is still valid when .text begins at 0x0.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Check function is GC'ed

[Pedro Alves]

On 08/06/2014 07:50 AM, Yao Qi wrote:
> @@ -17291,7 +17293,12 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
>  		case DW_LNE_set_address:
>  		  address = read_address (abfd, line_ptr, cu, &bytes_read);
>  
> -		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
> +		  if (!decode_for_pst_p)
> +		    pst = cu->per_cu->v.psymtab;
> +
> +		  if (address == 0
> +		      && (!dwarf2_per_objfile->has_section_at_zero
> +			  || (pst != NULL && pst->textlow > address)))
>  		    {
>  		      /* This line table is for a function which has been
>  			 GCd by the linker.  Ignore it.  PR gdb/12528 */

Does this still work when we have .gdb_index sections?
ISTR we don't have psymtabs in that case?

Thanks,
Pedro Alves

Re: [PATCH 1/2] Remove pst from dwarf_decode_lines_1

[Yao Qi]

On 08/06/2014 02:50 PM, Yao Qi wrote:
> gdb:
> 
> 2014-08-06  Yao Qi  <yao@codesourcery.com>
> 
> 	* dwarf2read.c (dwarf_decode_lines_1): Remove parameter 'pst'.
> 	Add parameter 'decode_for_pst_p'.  Callers update.

Ping.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Remove pst from dwarf_decode_lines_1

[Doug Evans]

On Wed, Aug 13, 2014 at 4:57 AM, Yao Qi <yao@codesourcery.com> wrote:
> On 08/06/2014 02:50 PM, Yao Qi wrote:
>> gdb:
>>
>> 2014-08-06  Yao Qi  <yao@codesourcery.com>
>>
>>       * dwarf2read.c (dwarf_decode_lines_1): Remove parameter 'pst'.
>>       Add parameter 'decode_for_pst_p'.  Callers update.
>
> Ping.

LGTM

Re: [PATCH 1/2] Remove pst from dwarf_decode_lines_1

[Yao Qi]

On 08/14/2014 02:58 AM, Doug Evans wrote:
> On Wed, Aug 13, 2014 at 4:57 AM, Yao Qi <yao@codesourcery.com> wrote:
>> On 08/06/2014 02:50 PM, Yao Qi wrote:
>>> gdb:
>>>
>>> 2014-08-06  Yao Qi  <yao@codesourcery.com>
>>>
>>>       * dwarf2read.c (dwarf_decode_lines_1): Remove parameter 'pst'.
>>>       Add parameter 'decode_for_pst_p'.  Callers update.
>>
>> Ping.
> 
> LGTM
> 

Patch is pushed in.  Thanks for the review.  Could you review
this patch too?

  [PATCH 2/2] Check function is GC'ed
  https://sourceware.org/ml/gdb-patches/2014-08/msg00067.html

They are in one series.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Remove pst from dwarf_decode_lines_1

[Yao Qi]

On 08/15/2014 07:50 AM, Yao Qi wrote:
> Could you review
> this patch too?
> 
>   [PATCH 2/2] Check function is GC'ed
>   https://sourceware.org/ml/gdb-patches/2014-08/msg00067.html
> 
> They are in one series.

Doug, if you haven't started review, could you please pause for a while?
I find some other places in dwarf2read.c need the similar fix too.  I'll
post an updated version.

-- 
Yao (齐尧)

[PATCH 1/3] Check function is GC'ed

[Yao Qi]

I see the following fail on arm-none-eabi target,

(gdb) b 24^M
Breakpoint 1 at 0x4: file
../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
line 24.^M
(gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24

Currently, we are using flag has_section_at_zero to determine whether
address zero in debug info means the corresponding code has been
GC'ed, like this:

	case DW_LNE_set_address:
	  address = read_address (abfd, line_ptr, cu, &bytes_read);

	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
	    {
	      /* This line table is for a function which has been
		 GCd by the linker.  Ignore it.  PR gdb/12528 */

However, this is incorrect on some bare metal targets, as .text
section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
is true.  If a function is GC'ed by linker, the address is zero.  GDB
thinks address zero is a function's address rather than this function
is GC'ed.

In this patch, we choose 'textlow' field of partial symtabl to check
whether 'textlow' is zero.  If it isn't, address zero really means the
function is GC'ed.  This patch fixes the fail above.  Note that this
patch only fixes the problem on the path that partial symtab is used.
On other paths partial symtab isn't used (start gdb with --readnow for
example), I don't find a good way to fix it.

It is regression tested on x86_64-linux, arm-none-eabi,
arm-none-linux-gnueabi.  OK to apply?

gdb:

2014-08-20  Yao Qi  <yao@codesourcery.com>

	* dwarf2read.c (dwarf_decode_lines_1): Skip the line table if
	PST->textlow is greater than zero.
---
 gdb/dwarf2read.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b4d53c8..cf2ce76 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17229,6 +17229,8 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
       /* Decode the table.  */
       while (!end_sequence)
 	{
+	  struct partial_symtab *pst = NULL;
+
 	  op_code = read_1_byte (abfd, line_ptr);
 	  line_ptr += 1;
 	  if (line_ptr > line_end)
@@ -17291,7 +17293,12 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 		case DW_LNE_set_address:
 		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 
-		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
+		  if (!decode_for_pst_p && !dwarf2_per_objfile->using_index)
+		    pst = cu->per_cu->v.psymtab;
+
+		  if (address == 0
+		      && (!dwarf2_per_objfile->has_section_at_zero
+			  || (pst != NULL && pst->textlow > address)))
 		    {
 		      /* This line table is for a function which has been
 			 GCd by the linker.  Ignore it.  PR gdb/12528 */
-- 
1.9.3

[PATCH 2/3] Check data is GC'ed

[Yao Qi]

We see the following fail on arm-none-eabi target,

(gdb) print &var^M
$1 = (int *) 0x0 <_ftext>^M
(gdb) FAIL: gdb.dwarf2/dw2-var-zero-addr.exp: print &var

Test dw2-var-zero-addr.exp is intended to test whether GDB can
correctly know variable var is GC'ed by linker.  Currently, the
heuristic GDB is using is (see add_partial_symbol)

 	  && addr == 0
	  && !dwarf2_per_objfile->has_section_at_zero

however, it doesn't work for bare metal targets, on which certain
section is located at address zero.  In this patch, we improve the
heuristic that if the variable address is zero, and section at address
zero is executable, we think the variable is GC'ed by linker, because
there can't be a variable in an executable section.  In order to know
this, we replace flag has_section_at_zero with a pointer
section_at_zero.

gdb:

2014-08-20  Yao Qi  <yao@codesourcery.com>

	* dwarf2read.c (struct dwarf2_per_objfile) <has_section_at_zero>:
	Remove.
	<section_at_zero>: New field.  Callers update.
	(add_partial_symbol): Extend the condition to check whether the
	section at zero is executable.
	(new_symbol_full): Check whether the section at zero is
	executable.
---
 gdb/dwarf2read.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index cf2ce76..f5b6341 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -278,9 +278,8 @@ struct dwarf2_per_objfile
      original data was compressed using 'dwz -m'.  */
   struct dwz_file *dwz_file;
 
-  /* A flag indicating wether this objfile has a section loaded at a
-     VMA of 0.  */
-  int has_section_at_zero;
+  /* The section of this objfile loaded at a VMA of 0.  */
+  asection *section_at_zero;
 
   /* True if we are using the mapped index,
      or we are faking it for OBJF_READNOW's sake.  */
@@ -2186,7 +2185,7 @@ dwarf2_locate_sections (bfd *abfd, asection *sectp, void *vnames)
 
   if ((bfd_get_section_flags (abfd, sectp) & SEC_LOAD)
       && bfd_section_vma (abfd, sectp) == 0)
-    dwarf2_per_objfile->has_section_at_zero = 1;
+    dwarf2_per_objfile->section_at_zero = sectp;
 }
 
 /* A helper function that decides whether a section is empty,
@@ -6851,7 +6850,13 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 
       if (pdi->d.locdesc
 	  && addr == 0
-	  && !dwarf2_per_objfile->has_section_at_zero)
+	  /* When the address is 0, if the object file doesn't have
+	     section at zero or the section at zero is executable,
+	     we think address 0 means the corresponding variable is
+	     removed by linker, instead of there is a data at address
+	     0.  */
+	  && (dwarf2_per_objfile->section_at_zero == NULL
+	      || dwarf2_per_objfile->section_at_zero->flags & SEC_CODE))
 	{
 	  /* A global or static variable may also have been stripped
 	     out by the linker if unused, in which case its address
@@ -7341,8 +7346,8 @@ dwarf2_read_symtab (struct partial_symtab *self,
 	    = objfile_data (objfile->separate_debug_objfile_backlink,
 			    dwarf2_objfile_data_key);
 
-	  dwarf2_per_objfile->has_section_at_zero
-	    = dpo_backlink->has_section_at_zero;
+	  dwarf2_per_objfile->section_at_zero
+	    = dpo_backlink->section_at_zero;
 	}
 
       dwarf2_per_objfile->reading_partial_symbols = 0;
@@ -11758,7 +11763,7 @@ dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return,
       /* A not-uncommon case of bad debug info.
 	 Don't pollute the addrmap with bad data.  */
       if (range_beginning + baseaddr == 0
-	  && !dwarf2_per_objfile->has_section_at_zero)
+	  && !dwarf2_per_objfile->section_at_zero)
 	{
 	  complaint (&symfile_complaints,
 		     _(".debug_ranges entry has start address of zero"
@@ -11871,7 +11876,7 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
      labels are not in the output, so the relocs get a value of 0.
      If this is a discarded function, mark the pc bounds as invalid,
      so that GDB will ignore it.  */
-  if (low == 0 && !dwarf2_per_objfile->has_section_at_zero)
+  if (low == 0 && !dwarf2_per_objfile->section_at_zero)
     return 0;
 
   *lowpc = low;
@@ -12095,7 +12100,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
 
 	      /* A not-uncommon case of bad debug info.
 		 Don't pollute the addrmap with bad data.  */
-	      if (start == 0 && !dwarf2_per_objfile->has_section_at_zero)
+	      if (start == 0 && !dwarf2_per_objfile->section_at_zero)
 		{
 		  complaint (&symfile_complaints,
 			     _(".debug_ranges entry has start address of zero"
@@ -15668,7 +15673,7 @@ read_partial_die (const struct die_reader_specs *reader,
 	 labels are not in the output, so the relocs get a value of 0.
 	 If this is a discarded function, mark the pc bounds as invalid,
 	 so that GDB will ignore it.  */
-      if (part_die->lowpc == 0 && !dwarf2_per_objfile->has_section_at_zero)
+      if (part_die->lowpc == 0 && !dwarf2_per_objfile->section_at_zero)
 	{
 	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
 
@@ -17297,7 +17302,7 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 		    pst = cu->per_cu->v.psymtab;
 
 		  if (address == 0
-		      && (!dwarf2_per_objfile->has_section_at_zero
+		      && (!dwarf2_per_objfile->section_at_zero
 			  || (pst != NULL && pst->textlow > address)))
 		    {
 		      /* This line table is for a function which has been
@@ -17870,7 +17875,8 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 
 	      if (SYMBOL_CLASS (sym) == LOC_STATIC
 		  && SYMBOL_VALUE_ADDRESS (sym) == 0
-		  && !dwarf2_per_objfile->has_section_at_zero)
+		  && (!dwarf2_per_objfile->section_at_zero
+		      || (dwarf2_per_objfile->section_at_zero->flags & SEC_CODE)))
 		{
 		  /* When a static variable is eliminated by the linker,
 		     the corresponding debug information is not stripped
-- 
1.9.3

Re: [PATCH 2/3] Check data is GC'ed

[Doug Evans]

Hi. Comments inline.

Yao Qi writes:
 > We see the following fail on arm-none-eabi target,
 > 
 > (gdb) print &var^M
 > $1 = (int *) 0x0 <_ftext>^M
 > (gdb) FAIL: gdb.dwarf2/dw2-var-zero-addr.exp: print &var
 > 
 > Test dw2-var-zero-addr.exp is intended to test whether GDB can
 > correctly know variable var is GC'ed by linker.  Currently, the
 > heuristic GDB is using is (see add_partial_symbol)
 > 
 >  	  && addr == 0
 > 	  && !dwarf2_per_objfile->has_section_at_zero
 > 
 > however, it doesn't work for bare metal targets, on which certain
 > section is located at address zero.  In this patch, we improve the
 > heuristic that if the variable address is zero, and section at address
 > zero is executable, we think the variable is GC'ed by linker, because
 > there can't be a variable in an executable section.  In order to know
 > this, we replace flag has_section_at_zero with a pointer
 > section_at_zero.
 > 
 > gdb:
 > 
 > 2014-08-20  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* dwarf2read.c (struct dwarf2_per_objfile) <has_section_at_zero>:
 > 	Remove.
 > 	<section_at_zero>: New field.  Callers update.
 > 	(add_partial_symbol): Extend the condition to check whether the
 > 	section at zero is executable.
 > 	(new_symbol_full): Check whether the section at zero is
 > 	executable.
 > ---
 >  gdb/dwarf2read.c | 32 +++++++++++++++++++-------------
 >  1 file changed, 19 insertions(+), 13 deletions(-)
 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index cf2ce76..f5b6341 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -278,9 +278,8 @@ struct dwarf2_per_objfile
 >       original data was compressed using 'dwz -m'.  */
 >    struct dwz_file *dwz_file;
 >  
 > -  /* A flag indicating wether this objfile has a section loaded at a
 > -     VMA of 0.  */
 > -  int has_section_at_zero;
 > +  /* The section of this objfile loaded at a VMA of 0.  */
 > +  asection *section_at_zero;

If overlays are in use, could multiple sections be at zero?
[I think so, but I haven't used overlays in awhile.]
Storing a vector of sections_at_zero seems like massive
overkill to solve this problem, maybe there's a simpler solution,
I don't know.  But I'm not opposed to it if it's the only
solution we have at the moment.

 >  
 >    /* True if we are using the mapped index,
 >       or we are faking it for OBJF_READNOW's sake.  */
 > @@ -2186,7 +2185,7 @@ dwarf2_locate_sections (bfd *abfd, asection *sectp, void *vnames)
 >  
 >    if ((bfd_get_section_flags (abfd, sectp) & SEC_LOAD)
 >        && bfd_section_vma (abfd, sectp) == 0)
 > -    dwarf2_per_objfile->has_section_at_zero = 1;
 > +    dwarf2_per_objfile->section_at_zero = sectp;
 >  }
 >  
 >  /* A helper function that decides whether a section is empty,
 > @@ -6851,7 +6850,13 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 >  
 >        if (pdi->d.locdesc
 >  	  && addr == 0
 > -	  && !dwarf2_per_objfile->has_section_at_zero)
 > +	  /* When the address is 0, if the object file doesn't have
 > +	     section at zero or the section at zero is executable,
 > +	     we think address 0 means the corresponding variable is
 > +	     removed by linker, instead of there is a data at address
 > +	     0.  */
 > +	  && (dwarf2_per_objfile->section_at_zero == NULL
 > +	      || dwarf2_per_objfile->section_at_zero->flags & SEC_CODE))

Could there be a DW_TAG_variable in SEC_CODE?
Someone might put one there for a particular reason.

 >  	{
 >  	  /* A global or static variable may also have been stripped
 >  	     out by the linker if unused, in which case its address
 > @@ -7341,8 +7346,8 @@ dwarf2_read_symtab (struct partial_symtab *self,
 >  	    = objfile_data (objfile->separate_debug_objfile_backlink,
 >  			    dwarf2_objfile_data_key);
 >  
 > -	  dwarf2_per_objfile->has_section_at_zero
 > -	    = dpo_backlink->has_section_at_zero;
 > +	  dwarf2_per_objfile->section_at_zero
 > +	    = dpo_backlink->section_at_zero;
 >  	}
 >  
 >        dwarf2_per_objfile->reading_partial_symbols = 0;
 > @@ -11758,7 +11763,7 @@ dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return,
 >        /* A not-uncommon case of bad debug info.
 >  	 Don't pollute the addrmap with bad data.  */
 >        if (range_beginning + baseaddr == 0
 > -	  && !dwarf2_per_objfile->has_section_at_zero)
 > +	  && !dwarf2_per_objfile->section_at_zero)

Convention is to write "ptr == NULL" instead of "!ptr".

 >  	{
 >  	  complaint (&symfile_complaints,
 >  		     _(".debug_ranges entry has start address of zero"
 > @@ -11871,7 +11876,7 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
 >       labels are not in the output, so the relocs get a value of 0.
 >       If this is a discarded function, mark the pc bounds as invalid,
 >       so that GDB will ignore it.  */
 > -  if (low == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +  if (low == 0 && !dwarf2_per_objfile->section_at_zero)
 >      return 0;
 >  
 >    *lowpc = low;
 > @@ -12095,7 +12100,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
 >  
 >  	      /* A not-uncommon case of bad debug info.
 >  		 Don't pollute the addrmap with bad data.  */
 > -	      if (start == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +	      if (start == 0 && !dwarf2_per_objfile->section_at_zero)
 >  		{
 >  		  complaint (&symfile_complaints,
 >  			     _(".debug_ranges entry has start address of zero"
 > @@ -15668,7 +15673,7 @@ read_partial_die (const struct die_reader_specs *reader,
 >  	 labels are not in the output, so the relocs get a value of 0.
 >  	 If this is a discarded function, mark the pc bounds as invalid,
 >  	 so that GDB will ignore it.  */
 > -      if (part_die->lowpc == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +      if (part_die->lowpc == 0 && !dwarf2_per_objfile->section_at_zero)
 >  	{
 >  	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
 >  
 > @@ -17297,7 +17302,7 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >  		    pst = cu->per_cu->v.psymtab;
 >  
 >  		  if (address == 0
 > -		      && (!dwarf2_per_objfile->has_section_at_zero
 > +		      && (!dwarf2_per_objfile->section_at_zero
 >  			  || (pst != NULL && pst->textlow > address)))
 >  		    {
 >  		      /* This line table is for a function which has been
 > @@ -17870,7 +17875,8 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 >  
 >  	      if (SYMBOL_CLASS (sym) == LOC_STATIC
 >  		  && SYMBOL_VALUE_ADDRESS (sym) == 0
 > -		  && !dwarf2_per_objfile->has_section_at_zero)
 > +		  && (!dwarf2_per_objfile->section_at_zero
 > +		      || (dwarf2_per_objfile->section_at_zero->flags & SEC_CODE)))

The SEC_CODE test doesn't feel right.
I understand the intent, but someone might put a variable in .text
for a reason.

 >  		{
 >  		  /* When a static variable is eliminated by the linker,
 >  		     the corresponding debug information is not stripped
 > -- 
 > 1.9.3
 > 

Re: [PATCH 2/3] Check data is GC'ed

[Yao Qi]

Doug Evans <dje@google.com> writes:

>  >        if (pdi->d.locdesc
>  >  	  && addr == 0
>  > -	  && !dwarf2_per_objfile->has_section_at_zero)
>  > +	  /* When the address is 0, if the object file doesn't have
>  > +	     section at zero or the section at zero is executable,
>  > +	     we think address 0 means the corresponding variable is
>  > +	     removed by linker, instead of there is a data at address
>  > +	     0.  */
>  > +	  && (dwarf2_per_objfile->section_at_zero == NULL
>  > +	      || dwarf2_per_objfile->section_at_zero->flags & SEC_CODE))
>
> Could there be a DW_TAG_variable in SEC_CODE?
> Someone might put one there for a particular reason.

I don't figure out a reason to do so, but it is possible via:

__attribute__((section(".text"))) int foo = 1;

I can't think of other heuristics, so if GDB has a section at address
zero, GDB can't tell address zero in debug information means the
corresponding variable is GC'ed by linker or the variable address is
zero.

How about open a PR about GDB is unable to know whether a variable is
GC'ed by linker if there is a section at address zero? and kfail
dw2-var-zero-addr.exp conditionally?  Something like:

    if [is_address_zero_readable] {
	setup_kfail "gdb/PR" "*-*-*"
    }
    gdb_test "print &var" {No symbol "var" in current context\.}

What do you think?

-- 
Yao (齐尧)

[PATCH 3/3] Run dw2-var-zero-addr.exp with --readnow

[Yao Qi]

This patch is to extend dw2-var-zero-add.exp to test the case that
partial symtabl is not used while full symtab is used, in order to
cover the changes in patch 2/3.  This patch restarts GDB with
--readnow and does the same test again.

gdb/testsuite:

2014-08-20  Yao Qi  <yao@codesourcery.com>

	* gdb.dwarf2/dw2-var-zero-addr.exp: Move test into new proc test.
	Invoke test.  Restart GDB with --readnow and invoke test again.
---
 gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp b/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp
index 462a5f8..5e1732b 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp
@@ -26,6 +26,20 @@ if [prepare_for_testing ${testfile}.exp ${testfile} \
     return -1
 }
 
-# FAIL was: = (int *) 0x0
-# Such DIE record can be produced using: gcc -fdata-sections -Wl,-gc-sections
-gdb_test "print &var" {No symbol "var" in current context\.}
+proc test { } {
+    # FAIL was: = (int *) 0x0
+    # Such DIE record can be produced using: gcc -fdata-sections -Wl,-gc-sections
+    gdb_test "print &var" {No symbol "var" in current context\.}
+}
+
+test
+
+global GDBFLAGS
+set saved_gdbflags $GDBFLAGS
+set GDBFLAGS "$GDBFLAGS --readnow"
+clean_restart ${binfile}
+set GDBFLAGS $saved_gdbflags
+
+with_test_prefix "readnow" {
+    test
+}
-- 
1.9.3

Re: [PATCH 3/3] Run dw2-var-zero-addr.exp with --readnow

[Doug Evans]

Yao Qi writes:
 > This patch is to extend dw2-var-zero-add.exp to test the case that
 > partial symtabl is not used while full symtab is used, in order to
 > cover the changes in patch 2/3.  This patch restarts GDB with
 > --readnow and does the same test again.
 > 
 > gdb/testsuite:
 > 
 > 2014-08-20  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* gdb.dwarf2/dw2-var-zero-addr.exp: Move test into new proc test.
 > 	Invoke test.  Restart GDB with --readnow and invoke test again.
 > ---
 >  gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp | 20 +++++++++++++++++---
 >  1 file changed, 17 insertions(+), 3 deletions(-)
 > 
 > diff --git a/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp b/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp
 > index 462a5f8..5e1732b 100644
 > --- a/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp
 > +++ b/gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.exp
 > @@ -26,6 +26,20 @@ if [prepare_for_testing ${testfile}.exp ${testfile} \
 >      return -1
 >  }
 >  
 > -# FAIL was: = (int *) 0x0
 > -# Such DIE record can be produced using: gcc -fdata-sections -Wl,-gc-sections
 > -gdb_test "print &var" {No symbol "var" in current context\.}
 > +proc test { } {
 > +    # FAIL was: = (int *) 0x0
 > +    # Such DIE record can be produced using: gcc -fdata-sections -Wl,-gc-sections
 > +    gdb_test "print &var" {No symbol "var" in current context\.}
 > +}
 > +
 > +test
 > +
 > +global GDBFLAGS

This global is unnecessary.
Otherwise the patch is fine by me.
[I kinda wonder whether the name "test" should be slightly more
descriptive, but I guess I'm ok with it as is.]

 > +set saved_gdbflags $GDBFLAGS
 > +set GDBFLAGS "$GDBFLAGS --readnow"
 > +clean_restart ${binfile}
 > +set GDBFLAGS $saved_gdbflags
 > +
 > +with_test_prefix "readnow" {
 > +    test
 > +}

Re: [PATCH 3/3] Run dw2-var-zero-addr.exp with --readnow

[Yao Qi]

Doug Evans <dje@google.com> writes:

>  > +global GDBFLAGS
>
> This global is unnecessary.
> Otherwise the patch is fine by me.

It is removed from the updated patch.  Pushed in.

-- 
Yao (齐尧)

Re: [PATCH 1/3] Check function is GC'ed

[Doug Evans]

Yao Qi writes:
 > I see the following fail on arm-none-eabi target,
 > 
 > (gdb) b 24^M
 > Breakpoint 1 at 0x4: file
 > ../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
 > line 24.^M
 > (gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24
 > 
 > Currently, we are using flag has_section_at_zero to determine whether
 > address zero in debug info means the corresponding code has been
 > GC'ed, like this:
 > 
 > 	case DW_LNE_set_address:
 > 	  address = read_address (abfd, line_ptr, cu, &bytes_read);
 > 
 > 	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > 	    {
 > 	      /* This line table is for a function which has been
 > 		 GCd by the linker.  Ignore it.  PR gdb/12528 */
 > 
 > However, this is incorrect on some bare metal targets, as .text
 > section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
 > is true.  If a function is GC'ed by linker, the address is zero.  GDB
 > thinks address zero is a function's address rather than this function
 > is GC'ed.
 > 
 > In this patch, we choose 'textlow' field of partial symtabl to check
 > whether 'textlow' is zero.  If it isn't, address zero really means the
 > function is GC'ed.  This patch fixes the fail above.  Note that this
 > patch only fixes the problem on the path that partial symtab is used.
 > On other paths partial symtab isn't used (start gdb with --readnow for
 > example), I don't find a good way to fix it.
 > 
 > It is regression tested on x86_64-linux, arm-none-eabi,
 > arm-none-linux-gnueabi.  OK to apply?
 > 
 > gdb:
 > 
 > 2014-08-20  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* dwarf2read.c (dwarf_decode_lines_1): Skip the line table if
 > 	PST->textlow is greater than zero.
 > ---
 >  gdb/dwarf2read.c | 9 ++++++++-
 >  1 file changed, 8 insertions(+), 1 deletion(-)
 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index b4d53c8..cf2ce76 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -17229,6 +17229,8 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >        /* Decode the table.  */
 >        while (!end_sequence)
 >  	{
 > +	  struct partial_symtab *pst = NULL;
 > +
 >  	  op_code = read_1_byte (abfd, line_ptr);
 >  	  line_ptr += 1;
 >  	  if (line_ptr > line_end)
 > @@ -17291,7 +17293,12 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >  		case DW_LNE_set_address:
 >  		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 >  
 > -		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +		  if (!decode_for_pst_p && !dwarf2_per_objfile->using_index)
 > +		    pst = cu->per_cu->v.psymtab;
 > +
 > +		  if (address == 0
 > +		      && (!dwarf2_per_objfile->has_section_at_zero
 > +			  || (pst != NULL && pst->textlow > address)))
 >  		    {
 >  		      /* This line table is for a function which has been
 >  			 GCd by the linker.  Ignore it.  PR gdb/12528 */

Hi.

I'd like to solve this for both partial syms and .gdb_index.

We want to, essentially, discard the entry if address is outside the
range of the cu (the range needn't be contiguous, but for the task
at hand I don't think it matters).  I haven't dug into this too deeply,
but if we have lowpc (which for example is that read_file_scope has
before it calls handle_DW_AT_stmt_list (which calls dwarf_decode_lines),
can we use that in the test?  Can we arrange for all callers of
dwarf_decode_lines_1 pass lowpc down to it? [Or both lowpc,highpc
for a more complete test, but for the task at hand we've only been
checking, effectively, lowpc all along so I don't mind leaving it at that
for now.]

Re: [PATCH 1/3] Check function is GC'ed

[Yao Qi]

Doug Evans <dje@google.com> writes:

> I'd like to solve this for both partial syms and .gdb_index.
>
> We want to, essentially, discard the entry if address is outside the
> range of the cu (the range needn't be contiguous, but for the task
> at hand I don't think it matters).  I haven't dug into this too deeply,
> but if we have lowpc (which for example is that read_file_scope has
> before it calls handle_DW_AT_stmt_list (which calls dwarf_decode_lines),
> can we use that in the test?  Can we arrange for all callers of
> dwarf_decode_lines_1 pass lowpc down to it? [Or both lowpc,highpc
> for a more complete test, but for the task at hand we've only been
> checking, effectively, lowpc all along so I don't mind leaving it at that
> for now.]

Hi Doug,
Patch below implemented what you suggested, if I understand you correct
:)

A new argument 'lowpc' is added to both dwarf2_build_include_psymtabs
and handle_DW_AT_stmt_list, to keep the similarity between them.  I
thought about merging them into one function, but still need more
thinking.

-- 
Yao (齐尧)

Subject: [PATCH] Check function is GC'ed

I see the following fail on arm-none-eabi target,

(gdb) b 24^M
Breakpoint 1 at 0x4: file
../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
line 24.^M
(gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24

Currently, we are using flag has_section_at_zero to determine whether
address zero in debug info means the corresponding code has been
GC'ed, like this:

	case DW_LNE_set_address:
	  address = read_address (abfd, line_ptr, cu, &bytes_read);

	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
	    {
	      /* This line table is for a function which has been
		 GCd by the linker.  Ignore it.  PR gdb/12528 */

However, this is incorrect on some bare metal targets, as .text
section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
is true.  If a function is GC'ed by linker, the address is zero.  GDB
thinks address zero is a function's address rather than this function
is GC'ed.

In this patch, we choose 'lowpc' got in read_file_scope to check
whether 'lowpc' is greater than zero.  If it isn't, address zero really
means the function is GC'ed.  In this patch, we pass 'lowpc' in
read_file_scope through handle_DW_AT_stmt_list and dwarf_decode_lines,
and to dwarf_decode_lines_1 finally.

This patch fixes the fail above. This patch also covers the path that
partial symbol isn't used, which is tested by starting gdb with
--readnow option.

It is regression tested on x86-linux with
target_board=dwarf4-gdb-index, and arm-none-eabi.  OK to apply?

gdb:

2014-08-27  Yao Qi  <yao@codesourcery.com>

	* dwarf2read.c (dwarf_decode_lines): Update declaration.
	(handle_DW_AT_stmt_list): Add argument 'lowpc'.  Update
	comments.  Callers update.
	(dwarf_decode_lines): Likewise.
	(dwarf2_build_include_psymtabs): Likewise.
	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
	comments.  Skip the line table if  'lowpc' is greater than
	'address'.

gdb/testsuite:

2014-08-27  Yao Qi  <yao@codesourcery.com>

	* gdb.base/break-on-linker-gcd-function.exp: Move test into new
	proc set_breakpoint_on_gcd_function.  Invoke
	set_breakpoint_on_gcd_function.  Restart GDB with --readnow and
	invoke set_breakpoint_on_gcd_function again.
---
 gdb/dwarf2read.c                                   | 35 ++++++++++++++--------
 .../gdb.base/break-on-linker-gcd-function.exp      | 24 +++++++++++----
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index be32309..8fdae74 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1512,7 +1512,8 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 						     struct dwarf2_cu *cu);
 
 static void dwarf_decode_lines (struct line_header *, const char *,
-				struct dwarf2_cu *, struct partial_symtab *);
+				struct dwarf2_cu *, struct partial_symtab *,
+				CORE_ADDR);
 
 static void dwarf2_start_subfile (const char *, const char *, const char *);
 
@@ -4436,7 +4437,8 @@ dwarf2_create_include_psymtab (const char *name, struct partial_symtab *pst,
 static void
 dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
 			       struct die_info *die,
-			       struct partial_symtab *pst)
+			       struct partial_symtab *pst,
+			       CORE_ADDR lowpc)
 {
   struct line_header *lh = NULL;
   struct attribute *attr;
@@ -4448,7 +4450,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
     return;  /* No linetable, so no includes.  */
 
   /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
-  dwarf_decode_lines (lh, pst->dirname, cu, pst);
+  dwarf_decode_lines (lh, pst->dirname, cu, pst, lowpc);
 
   free_line_header (lh);
 }
@@ -5954,7 +5956,7 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
 
   /* Get the list of files included in the current compilation unit,
      and build a psymtab for each of them.  */
-  dwarf2_build_include_psymtabs (cu, comp_unit_die, pst);
+  dwarf2_build_include_psymtabs (cu, comp_unit_die, pst, pst->textlow);
 
   if (dwarf2_read_debug)
     {
@@ -8967,11 +8969,12 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
 
 /* Handle DW_AT_stmt_list for a compilation unit.
    DIE is the DW_TAG_compile_unit die for CU.
-   COMP_DIR is the compilation directory.  */
+   COMP_DIR is the compilation directory.  LOWPC is passed to
+   dwarf_decode_lines.  See dwarf_decode_lines comments about it.  */
 
 static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
-			const char *comp_dir) /* ARI: editCase function */
+			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
   struct attribute *attr;
 
@@ -8988,7 +8991,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 	{
 	  cu->line_header = line_header;
 	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL);
+	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
 	}
     }
 }
@@ -9039,7 +9042,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
   /* Decode line number information if present.  We do this before
      processing child DIEs, so that the line header table is available
      for DW_AT_decl_file.  */
-  handle_DW_AT_stmt_list (die, cu, comp_dir);
+  handle_DW_AT_stmt_list (die, cu, comp_dir, lowpc);
 
   /* Process all dies in compilation unit.  */
   if (die->child != NULL)
@@ -17252,7 +17255,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
 
 static void
 dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
-		      struct dwarf2_cu *cu, const int decode_for_pst_p)
+		      struct dwarf2_cu *cu, const int decode_for_pst_p,
+		      CORE_ADDR lowpc)
 {
   const gdb_byte *line_ptr, *extended_end;
   const gdb_byte *line_end;
@@ -17375,7 +17379,9 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 		case DW_LNE_set_address:
 		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 
-		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
+		  if (address == 0
+		      && (!dwarf2_per_objfile->has_section_at_zero
+			  || lowpc > address))
 		    {
 		      /* This line table is for a function which has been
 			 GCd by the linker.  Ignore it.  PR gdb/12528 */
@@ -17595,17 +17601,20 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
    as the corresponding symtab.  Since COMP_DIR is not used in the name of the
    symtab we don't use it in the name of the psymtabs we create.
    E.g. expand_line_sal requires this when finding psymtabs to expand.
-   A good testcase for this is mb-inline.exp.  */
+   A good testcase for this is mb-inline.exp.
+
+   LOWPC is the lowest address in CU (or 0 if not known).  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
-		    struct dwarf2_cu *cu, struct partial_symtab *pst)
+		    struct dwarf2_cu *cu, struct partial_symtab *pst,
+		    CORE_ADDR lowpc)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
   struct subfile *first_subfile = current_subfile;
 
-  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p);
+  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {
diff --git a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
index e593b51..b8de1cd 100644
--- a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
+++ b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
@@ -41,9 +41,23 @@ if {[build_executable_from_specs $testfile.exp $testfile \
 
 clean_restart $testfile
 
-# Single hex digit
-set xd {[0-9a-f]}
+proc set_breakpoint_on_gcd_function {} {
+    # Single hex digit
+    set xd {[0-9a-f]}
+
+    # This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
+    # but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
+    gdb_test "b [gdb_get_line_number "gdb break here"]" \
+	"Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
+}
+
+set_breakpoint_on_gcd_function
 
-# This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
-# but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
-gdb_test "b [gdb_get_line_number "gdb break here"]" "Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
+set saved_gdbflags $GDBFLAGS
+set GDBFLAGS "$GDBFLAGS --readnow"
+clean_restart ${testfile}
+set GDBFLAGS $saved_gdbflags
+
+with_test_prefix "readnow" {
+    set_breakpoint_on_gcd_function
+}
-- 
1.9.3

Re: [PATCH 1/3] Check function is GC'ed

[Yao Qi]

Yao Qi <yao@codesourcery.com> writes:

> In this patch, we choose 'lowpc' got in read_file_scope to check
> whether 'lowpc' is greater than zero.  If it isn't, address zero really
> means the function is GC'ed.  In this patch, we pass 'lowpc' in
> read_file_scope through handle_DW_AT_stmt_list and dwarf_decode_lines,
> and to dwarf_decode_lines_1 finally.
>
> This patch fixes the fail above. This patch also covers the path that
> partial symbol isn't used, which is tested by starting gdb with
> --readnow option.
>
> It is regression tested on x86-linux with
> target_board=dwarf4-gdb-index, and arm-none-eabi.  OK to apply?
>
> gdb:
>
> 2014-08-27  Yao Qi  <yao@codesourcery.com>
>
> 	* dwarf2read.c (dwarf_decode_lines): Update declaration.
> 	(handle_DW_AT_stmt_list): Add argument 'lowpc'.  Update
> 	comments.  Callers update.
> 	(dwarf_decode_lines): Likewise.
> 	(dwarf2_build_include_psymtabs): Likewise.
> 	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
> 	comments.  Skip the line table if  'lowpc' is greater than
> 	'address'.
>
> gdb/testsuite:
>
> 2014-08-27  Yao Qi  <yao@codesourcery.com>
>
> 	* gdb.base/break-on-linker-gcd-function.exp: Move test into new
> 	proc set_breakpoint_on_gcd_function.  Invoke
> 	set_breakpoint_on_gcd_function.  Restart GDB with --readnow and
> 	invoke set_breakpoint_on_gcd_function again.

Ping.  https://sourceware.org/ml/gdb-patches/2014-08/msg00595.html

-- 
Yao (齐尧)

Re: [PATCH 1/3] Check function is GC'ed

[Doug Evans]

Yao Qi writes:
 > Doug Evans <dje@google.com> writes:
 > 
 > > I'd like to solve this for both partial syms and .gdb_index.
 > >
 > > We want to, essentially, discard the entry if address is outside the
 > > range of the cu (the range needn't be contiguous, but for the task
 > > at hand I don't think it matters).  I haven't dug into this too deeply,
 > > but if we have lowpc (which for example is that read_file_scope has
 > > before it calls handle_DW_AT_stmt_list (which calls dwarf_decode_lines),
 > > can we use that in the test?  Can we arrange for all callers of
 > > dwarf_decode_lines_1 pass lowpc down to it? [Or both lowpc,highpc
 > > for a more complete test, but for the task at hand we've only been
 > > checking, effectively, lowpc all along so I don't mind leaving it at that
 > > for now.]
 > 
 > Hi Doug,
 > Patch below implemented what you suggested, if I understand you correct
 > :)
 > 
 > A new argument 'lowpc' is added to both dwarf2_build_include_psymtabs
 > and handle_DW_AT_stmt_list, to keep the similarity between them.  I
 > thought about merging them into one function, but still need more
 > thinking.
 > 
 > -- 
 > Yao (  )
 > 
 > Subject: [PATCH] Check function is GC'ed
 > 
 > I see the following fail on arm-none-eabi target,
 > 
 > (gdb) b 24^M
 > Breakpoint 1 at 0x4: file
 > ../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
 > line 24.^M
 > (gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24
 > 
 > Currently, we are using flag has_section_at_zero to determine whether
 > address zero in debug info means the corresponding code has been
 > GC'ed, like this:
 > 
 > 	case DW_LNE_set_address:
 > 	  address = read_address (abfd, line_ptr, cu, &bytes_read);
 > 
 > 	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > 	    {
 > 	      /* This line table is for a function which has been
 > 		 GCd by the linker.  Ignore it.  PR gdb/12528 */
 > 
 > However, this is incorrect on some bare metal targets, as .text
 > section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
 > is true.  If a function is GC'ed by linker, the address is zero.  GDB
 > thinks address zero is a function's address rather than this function
 > is GC'ed.
 > 
 > In this patch, we choose 'lowpc' got in read_file_scope to check
 > whether 'lowpc' is greater than zero.  If it isn't, address zero really
 > means the function is GC'ed.  In this patch, we pass 'lowpc' in
 > read_file_scope through handle_DW_AT_stmt_list and dwarf_decode_lines,
 > and to dwarf_decode_lines_1 finally.
 > 
 > This patch fixes the fail above. This patch also covers the path that
 > partial symbol isn't used, which is tested by starting gdb with
 > --readnow option.
 > 
 > It is regression tested on x86-linux with
 > target_board=dwarf4-gdb-index, and arm-none-eabi.  OK to apply?
 > 
 > gdb:
 > 
 > 2014-08-27  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* dwarf2read.c (dwarf_decode_lines): Update declaration.
 > 	(handle_DW_AT_stmt_list): Add argument 'lowpc'.  Update
 > 	comments.  Callers update.
 > 	(dwarf_decode_lines): Likewise.
 > 	(dwarf2_build_include_psymtabs): Likewise.
 > 	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
 > 	comments.  Skip the line table if  'lowpc' is greater than
 > 	'address'.
 > 
 > gdb/testsuite:
 > 
 > 2014-08-27  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* gdb.base/break-on-linker-gcd-function.exp: Move test into new
 > 	proc set_breakpoint_on_gcd_function.  Invoke
 > 	set_breakpoint_on_gcd_function.  Restart GDB with --readnow and
 > 	invoke set_breakpoint_on_gcd_function again.

Hi.
Thanks for the ping.

 > ---
 >  gdb/dwarf2read.c                                   | 35 ++++++++++++++--------
 >  .../gdb.base/break-on-linker-gcd-function.exp      | 24 +++++++++++----
 >  2 files changed, 41 insertions(+), 18 deletions(-)
 > 
 > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
 > index be32309..8fdae74 100644
 > --- a/gdb/dwarf2read.c
 > +++ b/gdb/dwarf2read.c
 > @@ -1512,7 +1512,8 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 >  						     struct dwarf2_cu *cu);
 >  
 >  static void dwarf_decode_lines (struct line_header *, const char *,
 > -				struct dwarf2_cu *, struct partial_symtab *);
 > +				struct dwarf2_cu *, struct partial_symtab *,
 > +				CORE_ADDR);
 >  
 >  static void dwarf2_start_subfile (const char *, const char *, const char *);
 >  
 > @@ -4436,7 +4437,8 @@ dwarf2_create_include_psymtab (const char *name, struct partial_symtab *pst,
 >  static void
 >  dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
 >  			       struct die_info *die,
 > -			       struct partial_symtab *pst)
 > +			       struct partial_symtab *pst,
 > +			       CORE_ADDR lowpc)
 >  {
 >    struct line_header *lh = NULL;
 >    struct attribute *attr;
 > @@ -4448,7 +4450,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
 >      return;  /* No linetable, so no includes.  */
 >  
 >    /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
 > -  dwarf_decode_lines (lh, pst->dirname, cu, pst);
 > +  dwarf_decode_lines (lh, pst->dirname, cu, pst, lowpc);

We don't really need a "lowpc" argument to dwarf2_build_include_psymtabs.
Replace that with:

  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);

and remove the lowpc argument to dwarf2_build_include_psymtabs.

 >  
 >    free_line_header (lh);
 >  }
 > @@ -5954,7 +5956,7 @@ process_psymtab_comp_unit_reader (const struct die_reader_specs *reader,
 >  
 >    /* Get the list of files included in the current compilation unit,
 >       and build a psymtab for each of them.  */
 > -  dwarf2_build_include_psymtabs (cu, comp_unit_die, pst);
 > +  dwarf2_build_include_psymtabs (cu, comp_unit_die, pst, pst->textlow);
 >  
 >    if (dwarf2_read_debug)
 >      {
 > @@ -8967,11 +8969,12 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
 >  
 >  /* Handle DW_AT_stmt_list for a compilation unit.
 >     DIE is the DW_TAG_compile_unit die for CU.
 > -   COMP_DIR is the compilation directory.  */
 > +   COMP_DIR is the compilation directory.  LOWPC is passed to
 > +   dwarf_decode_lines.  See dwarf_decode_lines comments about it.  */
 >  
 >  static void
 >  handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 > -			const char *comp_dir) /* ARI: editCase function */
 > +			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 >  {
 >    struct attribute *attr;
 >  
 > @@ -8988,7 +8991,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 >  	{
 >  	  cu->line_header = line_header;
 >  	  make_cleanup (free_cu_line_header, cu);
 > -	  dwarf_decode_lines (line_header, comp_dir, cu, NULL);
 > +	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
 >  	}
 >      }
 >  }
 > @@ -9039,7 +9042,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
 >    /* Decode line number information if present.  We do this before
 >       processing child DIEs, so that the line header table is available
 >       for DW_AT_decl_file.  */
 > -  handle_DW_AT_stmt_list (die, cu, comp_dir);
 > +  handle_DW_AT_stmt_list (die, cu, comp_dir, lowpc);
 >  
 >    /* Process all dies in compilation unit.  */
 >    if (die->child != NULL)
 > @@ -17252,7 +17255,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
 >  
 >  static void
 >  dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 > -		      struct dwarf2_cu *cu, const int decode_for_pst_p)
 > +		      struct dwarf2_cu *cu, const int decode_for_pst_p,
 > +		      CORE_ADDR lowpc)
 >  {
 >    const gdb_byte *line_ptr, *extended_end;
 >    const gdb_byte *line_end;
 > @@ -17375,7 +17379,9 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >  		case DW_LNE_set_address:
 >  		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 >  
 > -		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > +		  if (address == 0
 > +		      && (!dwarf2_per_objfile->has_section_at_zero
 > +			  || lowpc > address))

I'm trying to decide whether to keep the
"!dwarf2_per_objfile->has_section_at_zero"
test here.  It feels wrong to keep it: What does it matter
whether any section has address zero?  It was only used as a proxy
for a better test.  Here we know we have an address
that is outside the CU in question, which is a far more specific test
than just checking whether any section is at address zero.
But maybe I'm missing something.

One could even argue that the "address == 0" test is also
now superfluous.

IOW, I'm wondering if we could just write the following here:

		  if (lowpc > address)

But if we write it that way then the code no longer readily expresses
what we're trying to do here which is handle the specific case expressed by
the comment that follows: "This line table is for a function which has been
GCd by the linker."  Instead we'd be now testing for a more general
case which would include bad debug info.

What do you think?

I'm leaning towards not changing things too much in this patch
and only handling GC'd functions.  Therefore I'm leaning towards
something like the following:

		  /* If address < lowpc then it's not a usable value, it's
		     outside the pc range of the CU.  However, we restrict
		     the test to only address values of zero to preserve
		     GDB's previous behaviour which is to handle the specific
		     case of a function being GC'd by the linker.  */
		  if (address == 0 && address < lowpc)

 >  		    {
 >  		      /* This line table is for a function which has been
 >  			 GCd by the linker.  Ignore it.  PR gdb/12528 */
 > @@ -17595,17 +17601,20 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 >     as the corresponding symtab.  Since COMP_DIR is not used in the name of the
 >     symtab we don't use it in the name of the psymtabs we create.
 >     E.g. expand_line_sal requires this when finding psymtabs to expand.
 > -   A good testcase for this is mb-inline.exp.  */
 > +   A good testcase for this is mb-inline.exp.
 > +
 > +   LOWPC is the lowest address in CU (or 0 if not known).  */
 >  
 >  static void
 >  dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
 > -		    struct dwarf2_cu *cu, struct partial_symtab *pst)
 > +		    struct dwarf2_cu *cu, struct partial_symtab *pst,
 > +		    CORE_ADDR lowpc)
 >  {
 >    struct objfile *objfile = cu->objfile;
 >    const int decode_for_pst_p = (pst != NULL);
 >    struct subfile *first_subfile = current_subfile;
 >  
 > -  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p);
 > +  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
 >  
 >    if (decode_for_pst_p)
 >      {
 > diff --git a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
 > index e593b51..b8de1cd 100644
 > --- a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
 > +++ b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
 > @@ -41,9 +41,23 @@ if {[build_executable_from_specs $testfile.exp $testfile \
 >  
 >  clean_restart $testfile
 >  
 > -# Single hex digit
 > -set xd {[0-9a-f]}
 > +proc set_breakpoint_on_gcd_function {} {
 > +    # Single hex digit
 > +    set xd {[0-9a-f]}
 > +
 > +    # This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
 > +    # but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
 > +    gdb_test "b [gdb_get_line_number "gdb break here"]" \
 > +	"Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
 > +}
 > +
 > +set_breakpoint_on_gcd_function
 >  
 > -# This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
 > -# but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
 > -gdb_test "b [gdb_get_line_number "gdb break here"]" "Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
 > +set saved_gdbflags $GDBFLAGS
 > +set GDBFLAGS "$GDBFLAGS --readnow"
 > +clean_restart ${testfile}
 > +set GDBFLAGS $saved_gdbflags
 > +
 > +with_test_prefix "readnow" {
 > +    set_breakpoint_on_gcd_function
 > +}

Re: [PATCH 1/3] Check function is GC'ed

[Yao Qi]

Doug Evans <dje@google.com> writes:

Thanks for the review, Doug.

>  >    /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
>  > -  dwarf_decode_lines (lh, pst->dirname, cu, pst);
>  > +  dwarf_decode_lines (lh, pst->dirname, cu, pst, lowpc);
>
> We don't really need a "lowpc" argument to dwarf2_build_include_psymtabs.
> Replace that with:
>
>   dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
>
> and remove the lowpc argument to dwarf2_build_include_psymtabs.
>

That is fine to me.  The reason I add "lowpc" argument to
dwarf2_build_include_psymtabs is that I'd like to keep the similarity
between dwarf2_build_include_psymtabs and handle_DW_AT_stmt_list.

>  > @@ -17252,7 +17255,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
>  >  
>  >  static void
>  >  dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
>  > -		      struct dwarf2_cu *cu, const int decode_for_pst_p)
>  > +		      struct dwarf2_cu *cu, const int decode_for_pst_p,
>  > +		      CORE_ADDR lowpc)
>  >  {
>  >    const gdb_byte *line_ptr, *extended_end;
>  >    const gdb_byte *line_end;
>  > @@ -17375,7 +17379,9 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
>  >  		case DW_LNE_set_address:
>  >  		  address = read_address (abfd, line_ptr, cu, &bytes_read);
>  >  
>  > -		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
>  > +		  if (address == 0
>  > +		      && (!dwarf2_per_objfile->has_section_at_zero
>  > +			  || lowpc > address))
>
> I'm trying to decide whether to keep the
> "!dwarf2_per_objfile->has_section_at_zero"
> test here.  It feels wrong to keep it: What does it matter
> whether any section has address zero?  It was only used as a proxy
> for a better test.  Here we know we have an address
> that is outside the CU in question, which is a far more specific test
> than just checking whether any section is at address zero.
> But maybe I'm missing something.

I thought about this when I wrote the patch.  I keep it in order to
preserve GDB's behaviour.  It should be OK to remove it.

>
> One could even argue that the "address == 0" test is also
> now superfluous.
>
> IOW, I'm wondering if we could just write the following here:
>
> 		  if (lowpc > address)
>
> But if we write it that way then the code no longer readily expresses
> what we're trying to do here which is handle the specific case expressed by
> the comment that follows: "This line table is for a function which has been
> GCd by the linker."  Instead we'd be now testing for a more general
> case which would include bad debug info.
>
> What do you think?
>
> I'm leaning towards not changing things too much in this patch
> and only handling GC'd functions.  Therefore I'm leaning towards
> something like the following:
>
> 		  /* If address < lowpc then it's not a usable value, it's
> 		     outside the pc range of the CU.  However, we restrict
> 		     the test to only address values of zero to preserve
> 		     GDB's previous behaviour which is to handle the specific
> 		     case of a function being GC'd by the linker.  */
> 		  if (address == 0 && address < lowpc)

I agree with you on this.  This patch is to fix a GDB bug when a
function is GC'ed by linker, we can concentrate on this at first.  We
can remove "address == 0" if it fixes some bugs we find in the future.

Patch below is updated to address your comments above.

-- 
Yao (齐尧)

From: Yao Qi <yao@codesourcery.com>
Subject: [PATCH] Check function is GC'ed

I see the following fail on arm-none-eabi target,

(gdb) b 24^M
Breakpoint 1 at 0x4: file
../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
line 24.^M
(gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24

Currently, we are using flag has_section_at_zero to determine whether
address zero in debug info means the corresponding code has been
GC'ed, like this:

	case DW_LNE_set_address:
	  address = read_address (abfd, line_ptr, cu, &bytes_read);

	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
	    {
	      /* This line table is for a function which has been
		 GCd by the linker.  Ignore it.  PR gdb/12528 */

However, this is incorrect on some bare metal targets, as .text
section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
is true.  If a function is GC'ed by linker, the address is zero.  GDB
thinks address zero is a function's address rather than this function
is GC'ed.

In this patch, we choose 'lowpc' got in read_file_scope to check
whether 'lowpc' is greater than zero.  If it isn't, address zero really
means the function is GC'ed.  In this patch, we pass 'lowpc' in
read_file_scope through handle_DW_AT_stmt_list and dwarf_decode_lines,
and to dwarf_decode_lines_1 finally.

This patch fixes the fail above. This patch also covers the path that
partial symbol isn't used, which is tested by starting gdb with
--readnow option.

It is regression tested on x86-linux with
target_board=dwarf4-gdb-index, and arm-none-eabi.  OK to apply?

gdb:

2014-09-16  Yao Qi  <yao@codesourcery.com>

	* dwarf2read.c (dwarf_decode_lines): Update declaration.
	(handle_DW_AT_stmt_list): Add argument 'lowpc'.  Update
	comments.  Callers update.
	(dwarf_decode_lines): Likewise.
	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
	comments.  Skip the line table if  'lowpc' is greater than
	'address'.  Don't check
	dwarf2_per_objfile->has_section_at_zero.

gdb/testsuite:

2014-09-16  Yao Qi  <yao@codesourcery.com>

	* gdb.base/break-on-linker-gcd-function.exp: Move test into new
	proc set_breakpoint_on_gcd_function.  Invoke
	set_breakpoint_on_gcd_function.  Restart GDB with --readnow and
	invoke set_breakpoint_on_gcd_function again.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index be32309..9d0ee13 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1512,7 +1512,8 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
 						     struct dwarf2_cu *cu);
 
 static void dwarf_decode_lines (struct line_header *, const char *,
-				struct dwarf2_cu *, struct partial_symtab *);
+				struct dwarf2_cu *, struct partial_symtab *,
+				CORE_ADDR);
 
 static void dwarf2_start_subfile (const char *, const char *, const char *);
 
@@ -4448,7 +4449,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
     return;  /* No linetable, so no includes.  */
 
   /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
-  dwarf_decode_lines (lh, pst->dirname, cu, pst);
+  dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
 
   free_line_header (lh);
 }
@@ -8967,11 +8968,12 @@ find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
 
 /* Handle DW_AT_stmt_list for a compilation unit.
    DIE is the DW_TAG_compile_unit die for CU.
-   COMP_DIR is the compilation directory.  */
+   COMP_DIR is the compilation directory.  LOWPC is passed to
+   dwarf_decode_lines.  See dwarf_decode_lines comments about it.  */
 
 static void
 handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
-			const char *comp_dir) /* ARI: editCase function */
+			const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
 {
   struct attribute *attr;
 
@@ -8988,7 +8990,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
 	{
 	  cu->line_header = line_header;
 	  make_cleanup (free_cu_line_header, cu);
-	  dwarf_decode_lines (line_header, comp_dir, cu, NULL);
+	  dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
 	}
     }
 }
@@ -9039,7 +9041,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
   /* Decode line number information if present.  We do this before
      processing child DIEs, so that the line header table is available
      for DW_AT_decl_file.  */
-  handle_DW_AT_stmt_list (die, cu, comp_dir);
+  handle_DW_AT_stmt_list (die, cu, comp_dir, lowpc);
 
   /* Process all dies in compilation unit.  */
   if (die->child != NULL)
@@ -17252,7 +17254,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
 
 static void
 dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
-		      struct dwarf2_cu *cu, const int decode_for_pst_p)
+		      struct dwarf2_cu *cu, const int decode_for_pst_p,
+		      CORE_ADDR lowpc)
 {
   const gdb_byte *line_ptr, *extended_end;
   const gdb_byte *line_end;
@@ -17375,7 +17378,12 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 		case DW_LNE_set_address:
 		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 
-		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
+		  /* If address < lowpc then it's not a usable value, it's
+		     outside the pc range of the CU.  However, we restrict
+		     the test to only address values of zero to preserve
+		     GDB's previous behaviour which is to handle the specific
+		     case of a function being GC'd by the linker.  */
+		  if (address == 0 && address < lowpc)
 		    {
 		      /* This line table is for a function which has been
 			 GCd by the linker.  Ignore it.  PR gdb/12528 */
@@ -17595,17 +17603,20 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
    as the corresponding symtab.  Since COMP_DIR is not used in the name of the
    symtab we don't use it in the name of the psymtabs we create.
    E.g. expand_line_sal requires this when finding psymtabs to expand.
-   A good testcase for this is mb-inline.exp.  */
+   A good testcase for this is mb-inline.exp.
+
+   LOWPC is the lowest address in CU (or 0 if not known).  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
-		    struct dwarf2_cu *cu, struct partial_symtab *pst)
+		    struct dwarf2_cu *cu, struct partial_symtab *pst,
+		    CORE_ADDR lowpc)
 {
   struct objfile *objfile = cu->objfile;
   const int decode_for_pst_p = (pst != NULL);
   struct subfile *first_subfile = current_subfile;
 
-  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p);
+  dwarf_decode_lines_1 (lh, comp_dir, cu, decode_for_pst_p, lowpc);
 
   if (decode_for_pst_p)
     {
diff --git a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
index e593b51..b8de1cd 100644
--- a/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
+++ b/gdb/testsuite/gdb.base/break-on-linker-gcd-function.exp
@@ -41,9 +41,23 @@ if {[build_executable_from_specs $testfile.exp $testfile \
 
 clean_restart $testfile
 
-# Single hex digit
-set xd {[0-9a-f]}
+proc set_breakpoint_on_gcd_function {} {
+    # Single hex digit
+    set xd {[0-9a-f]}
+
+    # This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
+    # but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
+    gdb_test "b [gdb_get_line_number "gdb break here"]" \
+	"Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
+}
+
+set_breakpoint_on_gcd_function
 
-# This accepts e.g. "Breakpoint 1 at 0x40968a" (fixed GDB)
-# but rejects e.g. "Breakpoint 1 at 0x4" (broken GDB).
-gdb_test "b [gdb_get_line_number "gdb break here"]" "Breakpoint \[0-9\] at 0x${xd}${xd}+: .*"
+set saved_gdbflags $GDBFLAGS
+set GDBFLAGS "$GDBFLAGS --readnow"
+clean_restart ${testfile}
+set GDBFLAGS $saved_gdbflags
+
+with_test_prefix "readnow" {
+    set_breakpoint_on_gcd_function
+}

Re: [PATCH 1/3] Check function is GC'ed

[Doug Evans]

Yao Qi writes:
 > Doug Evans <dje@google.com> writes:
 > 
 > Thanks for the review, Doug.
 > 
 > >  >    /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
 > >  > -  dwarf_decode_lines (lh, pst->dirname, cu, pst);
 > >  > +  dwarf_decode_lines (lh, pst->dirname, cu, pst, lowpc);
 > >
 > > We don't really need a "lowpc" argument to dwarf2_build_include_psymtabs.
 > > Replace that with:
 > >
 > >   dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
 > >
 > > and remove the lowpc argument to dwarf2_build_include_psymtabs.
 > >
 > 
 > That is fine to me.  The reason I add "lowpc" argument to
 > dwarf2_build_include_psymtabs is that I'd like to keep the similarity
 > between dwarf2_build_include_psymtabs and handle_DW_AT_stmt_list.
 > 
 > >  > @@ -17252,7 +17255,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
 > >  >  
 > >  >  static void
 > >  >  dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 > >  > -		      struct dwarf2_cu *cu, const int decode_for_pst_p)
 > >  > +		      struct dwarf2_cu *cu, const int decode_for_pst_p,
 > >  > +		      CORE_ADDR lowpc)
 > >  >  {
 > >  >    const gdb_byte *line_ptr, *extended_end;
 > >  >    const gdb_byte *line_end;
 > >  > @@ -17375,7 +17379,9 @@ dwarf_decode_lines_1 (struct line_header *lh, const char *comp_dir,
 > >  >  		case DW_LNE_set_address:
 > >  >  		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 > >  >  
 > >  > -		  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > >  > +		  if (address == 0
 > >  > +		      && (!dwarf2_per_objfile->has_section_at_zero
 > >  > +			  || lowpc > address))
 > >
 > > I'm trying to decide whether to keep the
 > > "!dwarf2_per_objfile->has_section_at_zero"
 > > test here.  It feels wrong to keep it: What does it matter
 > > whether any section has address zero?  It was only used as a proxy
 > > for a better test.  Here we know we have an address
 > > that is outside the CU in question, which is a far more specific test
 > > than just checking whether any section is at address zero.
 > > But maybe I'm missing something.
 > 
 > I thought about this when I wrote the patch.  I keep it in order to
 > preserve GDB's behaviour.  It should be OK to remove it.
 > 
 > >
 > > One could even argue that the "address == 0" test is also
 > > now superfluous.
 > >
 > > IOW, I'm wondering if we could just write the following here:
 > >
 > > 		  if (lowpc > address)
 > >
 > > But if we write it that way then the code no longer readily expresses
 > > what we're trying to do here which is handle the specific case expressed by
 > > the comment that follows: "This line table is for a function which has been
 > > GCd by the linker."  Instead we'd be now testing for a more general
 > > case which would include bad debug info.
 > >
 > > What do you think?
 > >
 > > I'm leaning towards not changing things too much in this patch
 > > and only handling GC'd functions.  Therefore I'm leaning towards
 > > something like the following:
 > >
 > > 		  /* If address < lowpc then it's not a usable value, it's
 > > 		     outside the pc range of the CU.  However, we restrict
 > > 		     the test to only address values of zero to preserve
 > > 		     GDB's previous behaviour which is to handle the specific
 > > 		     case of a function being GC'd by the linker.  */
 > > 		  if (address == 0 && address < lowpc)
 > 
 > I agree with you on this.  This patch is to fix a GDB bug when a
 > function is GC'ed by linker, we can concentrate on this at first.  We
 > can remove "address == 0" if it fixes some bugs we find in the future.
 > 
 > Patch below is updated to address your comments above.
 > 
 > -- 
 > Yao (  )
 > 
 > From: Yao Qi <yao@codesourcery.com>
 > Subject: [PATCH] Check function is GC'ed
 > 
 > I see the following fail on arm-none-eabi target,
 > 
 > (gdb) b 24^M
 > Breakpoint 1 at 0x4: file
 > ../../../../git/gdb/testsuite/gdb.base/break-on-linker-gcd-function.cc,
 > line 24.^M
 > (gdb) FAIL: gdb.base/break-on-linker-gcd-function.exp: b 24
 > 
 > Currently, we are using flag has_section_at_zero to determine whether
 > address zero in debug info means the corresponding code has been
 > GC'ed, like this:
 > 
 > 	case DW_LNE_set_address:
 > 	  address = read_address (abfd, line_ptr, cu, &bytes_read);
 > 
 > 	  if (address == 0 && !dwarf2_per_objfile->has_section_at_zero)
 > 	    {
 > 	      /* This line table is for a function which has been
 > 		 GCd by the linker.  Ignore it.  PR gdb/12528 */
 > 
 > However, this is incorrect on some bare metal targets, as .text
 > section is located at 0x0, so dwarf2_per_objfile->has_section_at_zero
 > is true.  If a function is GC'ed by linker, the address is zero.  GDB
 > thinks address zero is a function's address rather than this function
 > is GC'ed.
 > 
 > In this patch, we choose 'lowpc' got in read_file_scope to check
 > whether 'lowpc' is greater than zero.  If it isn't, address zero really
 > means the function is GC'ed.  In this patch, we pass 'lowpc' in
 > read_file_scope through handle_DW_AT_stmt_list and dwarf_decode_lines,
 > and to dwarf_decode_lines_1 finally.
 > 
 > This patch fixes the fail above. This patch also covers the path that
 > partial symbol isn't used, which is tested by starting gdb with
 > --readnow option.
 > 
 > It is regression tested on x86-linux with
 > target_board=dwarf4-gdb-index, and arm-none-eabi.  OK to apply?
 > 
 > gdb:
 > 
 > 2014-09-16  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* dwarf2read.c (dwarf_decode_lines): Update declaration.
 > 	(handle_DW_AT_stmt_list): Add argument 'lowpc'.  Update
 > 	comments.  Callers update.
 > 	(dwarf_decode_lines): Likewise.
 > 	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
 > 	comments.  Skip the line table if  'lowpc' is greater than
 > 	'address'.  Don't check
 > 	dwarf2_per_objfile->has_section_at_zero.
 > 
 > gdb/testsuite:
 > 
 > 2014-09-16  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* gdb.base/break-on-linker-gcd-function.exp: Move test into new
 > 	proc set_breakpoint_on_gcd_function.  Invoke
 > 	set_breakpoint_on_gcd_function.  Restart GDB with --readnow and
 > 	invoke set_breakpoint_on_gcd_function again.

LGTM.

Re: [PATCH 1/3] Check function is GC'ed

[Yao Qi]

Doug Evans <dje@google.com> writes:

>  > gdb:
>  > 
>  > 2014-09-16  Yao Qi  <yao@codesourcery.com>
>  > 
>  > 	* dwarf2read.c (dwarf_decode_lines): Update declaration.
>  > 	(handle_DW_AT_stmt_list): Add argument 'lowpc'.  Update
>  > 	comments.  Callers update.
>  > 	(dwarf_decode_lines): Likewise.
>  > 	(dwarf_decode_lines_1): Add argument 'lowpc'.  Update
>  > 	comments.  Skip the line table if  'lowpc' is greater than
>  > 	'address'.  Don't check
>  > 	dwarf2_per_objfile->has_section_at_zero.
>  > 
>  > gdb/testsuite:
>  > 
>  > 2014-09-16  Yao Qi  <yao@codesourcery.com>
>  > 
>  > 	* gdb.base/break-on-linker-gcd-function.exp: Move test into new
>  > 	proc set_breakpoint_on_gcd_function.  Invoke
>  > 	set_breakpoint_on_gcd_function.  Restart GDB with --readnow and
>  > 	invoke set_breakpoint_on_gcd_function again.
>
> LGTM.

Thanks for the review.  Patch is pushed in.

-- 
Yao (齐尧)