[PATCH 0/8] Non-contiguous address range support

[PATCH 0/8] Non-contiguous address range support

[Kevin Buettner]

This eight part patch sequence adds (further) support for
non-contiguous address ranges to GDB.

This sequence of patches was motivated by GCC bug 84550:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84550

There is a test case posted to that bug along with some analysis of
the underlying problem.

There is also a GDB bug for the same issue; it's 23021, but at the
moment, there is little there aside from a link to the GCC bug
mentioned above.  But here's a link anyway:

https://sourceware.org/bugzilla/show_bug.cgi?id=23021

A quick synopsis of the problem is as follows...

Recent versions of gcc can generate code in which a function is split
into at least two non-contiguous address ranges.  As I understand it,
the idea here is to separate code which gcc does not expect to execute
in normal operation from the rest of the code.  Doing this may result
in better cache locality for the normal case.  The generated code for
the example in GCC bug 84550 separated a call to abort() from the rest
of the code comprising the function.

In the course of my investigation, I identified at least four
problems:

1) Stepping into a function from a function which occupies non-contiguous
   address ranges does not always work.  It was not uncommon to see the
   program run to completion when attempting to do a step.

2) Setting a breakpoint on a function with non-contiguous address ranges
   causes a breakpoint to be placed on more than one location.  When a
   breakpoint is set on the "cold" address range, this is almost certainly
   incorrect.  The breakpoint should instead be set only on code near the
   entry point(s).

3) The disassemble command did not work correctly.  E.g. here is what I
   found during my analysis of 84550:

	(gdb) x/i 'main.cold.0'
	   0x4010e0 <main()>:   mov    %rax,%rdi
	(gdb) x/i main
	   0x4011a0 <main>:     push   %r12
	(gdb) disassemble main
	Dump of assembler code for function main():
	   0x00000000004010e0 <+0>:     mov    %rax,%rdi
	   ...
        [No addresses starting at 0x4011a0 are shown]

4) Display of addresses associated with the non-contiguous function are
   confusing.  E.g. in the above example, note that GDB thinks that
   the address associated with main.cold.0 is <main()>, but that there's
   also a minsym called main which is displayed as <main>.

There are probably several other problems which are related those
identified above.

I discovered that the stepping problem could be "fixed" by disabling
the find_pc_partial_function cache.  This cache keeps track of the
most recent result (of calling find_pc_partial_function).  If
find_pc_partial_function is called with an address which falls within
the cache range, then that's considered to be a cache hit and the most
recent result is returned.  Obviously, this won't work correctly for
functions which occupy non-contiguous (disjoint) address ranges where
other functions might be placed in the gap.

So one of the problems that needed to be solved was to make the
caching code work correctly.  It is interesting to note that stepping
_did_ work when the cache was disabled.  This is/was due to GDB
already having some (albeit incomplete) support for non-contiguous
addresses in the form of blockvector address maps.  Code responsible
for mapping addresses to blocks (which form the lower levels of
find_pc_partial_function) handle this case correctly.

To solve the problem of incorrect disassembly, we need to be able
to iterate over all of the ranges associated with a block.

Finally, we need to distinguish between the entry pc and the lowest
address in a block.  I discovered that this lack of distinction was
the cause of the remainder of the bugs including some which seemed to
be introduced by fixing the problems noted above.  Once this
distinction is made, it will be straightforward to add full support for
DW_AT_entry_pc.  I considered adding this support as part of this
patch series, but decided to wait until the community weighs in on my
work thus far...

[PATCH 1/8] Add block range data structure for blocks with non-contiguous address ranges

[Kevin Buettner]

This patch does the following:

- Introduces a block range data structure which is accessed via
  a new field in struct block.
- Defines several macros for accessing block ranges.
- Defines a new function, make_blockrange, which is responsible for
  creating the new data structure.
- Defines a predicate (function) which checks to see if an address
  is present in a block.

It should be noted that some support for non-contiguous ranges already
existed in GDB in the form of blockvector addrmaps.  This support
allowed GDB to quickly find a block containing a particular address
even when the block consists of non-contiguous addresses.  See
find_block_in_blockvector() in block.c, dwarf2_record_block_ranges()
in dwarf2read.c, and record_block_range() in buildsym.c.

Addrmaps do not provide a convenient way to examine address ranges
associated with a particular block.  This data structure (and its
interface) is set up for quickly finding the value (which in this case
is a block) associated with a particular address.  The interface
does not include a method for doing a reverse mapping from blocks to
addresses.  A linear time mapping might be attempted via use of the
addrmap's foreach method, but this is not as straightforward as it
might first appear due to the fact that blocks corresponding to inline
function instances and lexical blocks w/ variables end up getting
interspersed in in the set of transitions.

Note:  If this approach is deemed to be too expensive in terms of
space, an alternate approach might be to attempt the linear time
mapping noted above.  find_pc_partial_function() needs to be able to
quickly know whether there are discontiguous ranges (for determining
cache validity), so a flag for this property would have to be added to
struct block.  Also integral to this set of changes is the concept of
an "entry pc" which might be different than the block's start address.
An entry_pc field would also need to be added to struct block.  This
does not result in any space savings in struct block though since the
space for the flag and entry_pc use more space than the blockranges
struct pointer that I've added.  There would, however, be some space
savings due to the fact that the new data structures that I've added
for this patch would not need to be allocated.  (I happen to like the
approach I've come up with, but I wanted to mention another possibility
just in case someone does not.)

gdb/ChangeLog:
    
    	* block.h (blockrange, blockranges): New struct declarations.
    	(struct block): Add new field named `ranges'.
    	(BLOCK_RANGES, BLOCK_NRANGES, BLOCK_RANGE, BLOCK_CONTIGUOUS_P)
    	(BLOCK_RANGE_START, BLOCK_RANGE_END, BLOCK_ENTRY_PC): New
    	macros for accessing ranges in struct block.
    	(make_blockranges): New declaration.
    	(block_contains_pc): New declaration.
    	block.c (make_blockranges): New function.
    	(block_contains_pc): New function.
---
 gdb/block.c | 39 ++++++++++++++++++++++++++++
 gdb/block.h | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+)

diff --git a/gdb/block.c b/gdb/block.c
index f26d169..43234ee 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -65,6 +65,21 @@ block_gdbarch (const struct block *block)
   return get_objfile_arch (block_objfile (block));
 }
 
+/* See block.h.  */
+
+bool
+block_contains_pc (const struct block *b, CORE_ADDR pc)
+{
+  if (BLOCK_CONTIGUOUS_P (b))
+    return (BLOCK_START (b) <= pc && pc < BLOCK_END (b));
+  else
+    for (int i = 0; i < BLOCK_NRANGES (b); i++)
+      if (BLOCK_RANGE_START (b, i) <= pc && pc < BLOCK_RANGE_END (b, i))
+        return true;
+
+  return false;
+}
+
 /* Return Nonzero if block a is lexically nested within block b,
    or if a and b have the same pc range.
    Return zero otherwise.  */
@@ -807,3 +822,27 @@ block_find_non_opaque_type_preferred (struct symbol *sym, void *data)
   *best = sym;
   return 0;
 }
+
+/* See block.h.  */
+
+struct blockranges *
+make_blockranges (struct objfile * objfile,
+                  std::vector<std::pair<CORE_ADDR, CORE_ADDR>> &rangevec)
+{
+  struct blockranges *blr;
+  size_t n = rangevec.size();
+
+  blr = (struct blockranges *)
+    obstack_alloc (&objfile->objfile_obstack,
+                   sizeof (struct blockranges)
+		   + (n - 1) * sizeof (struct blockrange));
+
+  blr->nranges = n;
+  for (int i = 0; i < n; i++)
+    {
+      blr->range[i].startaddr = rangevec[i].first;
+      blr->range[i].endaddr = rangevec[i].second;
+    }
+  return blr;
+}
+
diff --git a/gdb/block.h b/gdb/block.h
index ff12725..ec89b62 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -31,6 +31,32 @@ struct using_direct;
 struct obstack;
 struct addrmap;
 
+/* Blocks can occupy non-contiguous address ranges.  When this occurs,
+   startaddr and endaddr within struct block (still) specify the lowest
+   and highest addresses of all ranges, but each individual range is
+   specified by the addresses in struct blockrange.  */
+
+struct blockrange
+{
+
+  /* Lowest address in this range.  */
+
+  CORE_ADDR startaddr;
+
+  /* One past the highest address in the range.  */
+
+  CORE_ADDR endaddr;
+};
+
+/* Two or more non-contiguous ranges in the same order as that provided
+   via the debug info.  */
+
+struct blockranges
+{
+  int nranges;
+  struct blockrange range[1];
+};
+
 /* All of the name-scope contours of the program
    are represented by `struct block' objects.
    All of these objects are pointed to by the blockvector.
@@ -86,6 +112,12 @@ struct block
      using directives and the current namespace scope.  */
 
   struct block_namespace_info *namespace_info;
+
+  /* Address ranges for blocks with non-contiguous ranges.  If this
+     is NULL, then there is only one range which is specified by
+     startaddr and endaddr above.  */
+
+  struct blockranges *ranges;
 };
 
 /* The global block is singled out so that we can provide a back-link
@@ -109,6 +141,49 @@ struct global_block
 #define BLOCK_DICT(bl)		(bl)->dict
 #define BLOCK_NAMESPACE(bl)	(bl)->namespace_info
 
+/* Accessor for ranges field within block BL.  */
+
+#define BLOCK_RANGES(bl)	(bl)->ranges
+
+/* Number of ranges within a block.  */
+
+#define BLOCK_NRANGES(bl)	(bl)->ranges->nranges
+
+/* Access range array for block BL.  */
+
+#define BLOCK_RANGE(bl)		(bl)->ranges->range
+
+/* Are all addresses within a block contiguous?  */
+
+#define BLOCK_CONTIGUOUS_P(bl)	(BLOCK_RANGES (bl) == nullptr \
+				 || BLOCK_NRANGES (bl) <= 1)
+
+/* Obtain the start address of the Nth range for block BL.  */
+
+#define BLOCK_RANGE_START(bl,n) (BLOCK_RANGE (bl)[n].startaddr)
+
+/* Obtain the end address of the Nth range for block BL.  */
+
+#define BLOCK_RANGE_END(bl,n)	(BLOCK_RANGE (bl)[n].endaddr)
+
+/* Define the "entry pc" for a block BL to be the lowest (start) address
+   for the block when all addresses within the block are contiguous.  If
+   non-contiguous, then use the start address for the first range in the
+   block.
+
+   At the moment, this almost matches what DWARF specifies as the entry
+   pc.  (The missing bit is support for DW_AT_entry_pc which should be
+   preferred over range data and the low_pc.)
+
+   Once support for DW_AT_entry_pc is added, I expect that an entry_pc
+   field will be added to one of these data structures.  Once that's done,
+   the entry_pc field can be set from the dwarf reader (and other readers
+   too).  BLOCK_ENTRY_PC can then be redefined to be less DWARF-centric.  */
+
+#define BLOCK_ENTRY_PC(bl)	(BLOCK_CONTIGUOUS_P (bl) \
+				 ? BLOCK_START (bl) \
+				 : BLOCK_RANGE_START (bl,0))
+
 struct blockvector
 {
   /* Number of blocks in the list.  */
@@ -139,6 +214,10 @@ extern struct symbol *block_containing_function (const struct block *);
 
 extern int block_inlined_p (const struct block *block);
 
+/* Return true if PC is in block BLOCK, false otherwise.  */
+
+extern bool block_contains_pc (const struct block *block, CORE_ADDR pc);
+
 extern int contained_in (const struct block *, const struct block *);
 
 extern const struct blockvector *blockvector_for_pc (CORE_ADDR,
@@ -322,4 +401,9 @@ extern int block_find_non_opaque_type_preferred (struct symbol *sym,
        (sym) != NULL;							\
        (sym) = block_iter_match_next ((name), &(iter)))
 
+/* Given a vector of pairs, allocate and build an obstack allocated
+   blockranges struct for a block.  */
+struct blockranges *make_blockranges (struct objfile *objfile,
+                                      std::vector<std::pair<CORE_ADDR, CORE_ADDR>> &);
+
 #endif /* BLOCK_H */

[PATCH 2/8] Record explicit block ranges from dwarf2read.c

[Kevin Buettner]

This change sets BLOCK_RANGES for the block under consideration by
calling make_blockranges().  This action is performed in
dwarf2_record_block_ranges().

It should be noted that dwarf2_record_block_ranges() already does some
recording of the range via a call to record_block_range().  The ranges
recorded in that fashion end up in the address map associated with the
blockvector for the compilation unit's symtab.  Given an address, the
addrmap provides a fast way of finding the block containing that
address.  The address map does not, however, provide a convenient way
of determining which address ranges make up a particular block.

While reading a set of ranges, a vector of pairs is used to collect
the starting and ending addresses for each range in the block.  Once
all of the ranges for a block have been collected, make_blockranges()
is called to fill in BLOCK_RANGES for the block.

The ranges are stored for the block in the order that they're read
from the debug info.  For DWARF, the starting address of the first
range of the block will be the entry pc in cases where DW_AT_entry_pc
is not present.  (Well, that would ideally be the case.  At the moment
DW_AT_entry_pc is not being handled.)

gdb/ChangeLog:
    
    	* dwarf2read.c (dwarf2_record_block_ranges): Fill in BLOCK_RANGES
    	for block.
---
 gdb/dwarf2read.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 4ad0527..2f0f9b9 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -14722,6 +14722,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
       unsigned long offset = (DW_UNSND (attr)
 			      + (need_ranges_base ? cu->ranges_base : 0));
 
+      std::vector<std::pair<CORE_ADDR, CORE_ADDR>> blockvec;
       dwarf2_ranges_process (offset, cu,
 	[&] (CORE_ADDR start, CORE_ADDR end)
 	{
@@ -14730,7 +14731,10 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
 	  start = gdbarch_adjust_dwarf2_addr (gdbarch, start);
 	  end = gdbarch_adjust_dwarf2_addr (gdbarch, end);
 	  record_block_range (block, start, end - 1);
+	  blockvec.push_back (std::make_pair (start, end));
 	});
+
+      BLOCK_RANGES(block) = make_blockranges (objfile, blockvec);
     }
 }
 

[PATCH 3/8] Add support for non-contiguous blocks to find_pc_partial_function

[Kevin Buettner]

This change adds an optional output parameter BLOCK to
find_pc_partial_function.  If BLOCK is non-null, then *BLOCK will be
set to the address of the block corresponding to the function symbol
if such a symbol was found during lookup.  Otherwise it's set to a
NULL value.  Callers may wish to use the block information to
determine whether the block contains any non-contiguous ranges.  The
caller may also iterate over or examine those ranges.

When I first started looking at the broken stepping behavior associated
with functions w/ non-contiguous ranges, I found that I could "fix"
the problem by disabling the find_pc_partial_function cache.  It would
sometimes happen that the PC passed in would be between the low and
high cache values, but would be in some other function that happens to
be placed in between the ranges for the cached function.  This caused
incorrect values to be returned.

So dealing with this cache turns out to be very important for fixing
this problem.

I explored two different ways of dealing with the cache.  My first
approach was to clear the cache when a block was encountered with
more than one range.  This would cause the non-cache pathway to be
executed on the next call to find_pc_partial_function.

My current approach, which I suspect is slightly faster, checks to see
whether the PC is within one of the ranges associated with the cached
block.  If so, then the cached values can be used.  (It falls back to
the original behavior if there is no cached block.)

Another choice that had to be made was whether to have ADDRESS
continue to represent the lowest address associated with the function
or with the entry pc associated with the function.  For the moment,
I've decided to keep the current behavior of having it represent the
lowest address.  In cases where the entry pc is needed, this can be
found by passing a non-NULL value for BLOCK which will cause the block
(pointer) associated with the function to be returned.  BLOCK_ENTRY_PC
can then be used on that block to obtain the entry pc.

gdb/ChangeLog:
    
    	* symtab.h (find_pc_partial_function): Add new parameter `block'.
    	* blockframe.c (cache_pc_function_block): New static global.
    	(clear_pc_function_cache): Clear cache_pc_function_block.
    	(find_pc_partial_function): Move comment to symtab.h. Add support
    	for non-contiguous blocks.
---
 gdb/blockframe.c | 35 ++++++++++++++++++-----------------
 gdb/symtab.h     | 20 ++++++++++++++++----
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index b3c9aa3..3892c46 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -159,6 +159,7 @@ static CORE_ADDR cache_pc_function_low = 0;
 static CORE_ADDR cache_pc_function_high = 0;
 static const char *cache_pc_function_name = 0;
 static struct obj_section *cache_pc_function_section = NULL;
+static const struct block *cache_pc_function_block = nullptr;
 
 /* Clear cache, e.g. when symbol table is discarded.  */
 
@@ -169,24 +170,14 @@ clear_pc_function_cache (void)
   cache_pc_function_high = 0;
   cache_pc_function_name = (char *) 0;
   cache_pc_function_section = NULL;
+  cache_pc_function_block = nullptr;
 }
 
-/* Finds the "function" (text symbol) that is smaller than PC but
-   greatest of all of the potential text symbols in SECTION.  Sets
-   *NAME and/or *ADDRESS conditionally if that pointer is non-null.
-   If ENDADDR is non-null, then set *ENDADDR to be the end of the
-   function (exclusive), but passing ENDADDR as non-null means that
-   the function might cause symbols to be read.  This function either
-   succeeds or fails (not halfway succeeds).  If it succeeds, it sets
-   *NAME, *ADDRESS, and *ENDADDR to real information and returns 1.
-   If it fails, it sets *NAME, *ADDRESS and *ENDADDR to zero and
-   returns 0.  */
-
-/* Backward compatibility, no section argument.  */
+/* See symtab.h.  */
 
 int
 find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
-			  CORE_ADDR *endaddr)
+			  CORE_ADDR *endaddr, const struct block **block)
 {
   struct obj_section *section;
   struct symbol *f;
@@ -206,9 +197,13 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
 
   mapped_pc = overlay_mapped_address (pc, section);
 
-  if (mapped_pc >= cache_pc_function_low
-      && mapped_pc < cache_pc_function_high
-      && section == cache_pc_function_section)
+  if ((!cache_pc_function_block
+       && mapped_pc >= cache_pc_function_low
+       && mapped_pc < cache_pc_function_high
+       && section == cache_pc_function_section)
+      || (cache_pc_function_block
+          && block_contains_pc (cache_pc_function_block, mapped_pc)))
+
     goto return_cached_value;
 
   msymbol = lookup_minimal_symbol_by_pc_section (mapped_pc, section);
@@ -232,13 +227,15 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
       f = find_pc_sect_function (mapped_pc, section);
       if (f != NULL
 	  && (msymbol.minsym == NULL
-	      || (BLOCK_START (SYMBOL_BLOCK_VALUE (f))
+	      || (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (f))
 		  >= BMSYMBOL_VALUE_ADDRESS (msymbol))))
 	{
 	  cache_pc_function_low = BLOCK_START (SYMBOL_BLOCK_VALUE (f));
 	  cache_pc_function_high = BLOCK_END (SYMBOL_BLOCK_VALUE (f));
 	  cache_pc_function_name = SYMBOL_LINKAGE_NAME (f);
 	  cache_pc_function_section = section;
+	  cache_pc_function_block = SYMBOL_BLOCK_VALUE (f);
+
 	  goto return_cached_value;
 	}
     }
@@ -268,6 +265,7 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
   cache_pc_function_name = MSYMBOL_LINKAGE_NAME (msymbol.minsym);
   cache_pc_function_section = section;
   cache_pc_function_high = minimal_symbol_upper_bound (msymbol);
+  cache_pc_function_block = nullptr;
 
  return_cached_value:
 
@@ -298,6 +296,9 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
 	*endaddr = cache_pc_function_high;
     }
 
+  if (block)
+    *block = cache_pc_function_block;
+
   return 1;
 }
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 84fc897..e4de868 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1683,10 +1683,22 @@ extern struct symbol *find_pc_sect_function (CORE_ADDR, struct obj_section *);
 
 extern struct symbol *find_symbol_at_address (CORE_ADDR);
 
-/* lookup function from address, return name, start addr and end addr.  */
-
-extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *,
-				     CORE_ADDR *);
+/* Finds the "function" (text symbol) that is smaller than PC but
+   greatest of all of the potential text symbols in SECTION.  Sets
+   *NAME and/or *ADDRESS conditionally if that pointer is non-null.
+   If ENDADDR is non-null, then set *ENDADDR to be the end of the
+   function (exclusive).  If the optional parameter BLOCK is non-null,
+   then set *BLOCK to the address of the block corresponding to the
+   function symbol, if such a symbol could be found during the lookup;
+   nullptr is used as a return value for *BLOCK if no block is found. 
+   This function either succeeds or fails (not halfway succeeds).  If
+   it succeeds, it sets *NAME, *ADDRESS, and *ENDADDR to real
+   information and returns 1.  If it fails, it sets *NAME, *ADDRESS
+   and *ENDADDR to zero and returns 0.  */
+
+extern int find_pc_partial_function (CORE_ADDR pc, const char **name,
+				     CORE_ADDR *address, CORE_ADDR *endaddr,
+				     const struct block **block = nullptr);
 
 /* Return the type of a function with its first instruction exactly at
    the PC address.  Return NULL otherwise.  */

[PATCH 4/8] Disassemble blocks with non-contiguous ranges

[Kevin Buettner]

This patch adds support for disassembly of blocks with non-contiguous
ranges.  These blocks are printed as follows:

(gdb) disassemble foo
Dump of assembler code for function foo:
Address range 0x401136 to 0x401151:
   0x0000000000401136 <+0>:     push   %rbp
   0x0000000000401137 <+1>:     mov    %rsp,%rbp
   0x000000000040113a <+4>:     callq  0x401134 <bar>
   0x000000000040113f <+9>:     mov    0x2eef(%rip),%eax        # 0x404034 <e>
   0x0000000000401145 <+15>:    test   %eax,%eax
   0x0000000000401147 <+17>:    je     0x40114e <foo+24>
   0x0000000000401149 <+19>:    callq  0x401128 <foo+4294967282>
   0x000000000040114e <+24>:    nop
   0x000000000040114f <+25>:    pop    %rbp
   0x0000000000401150 <+26>:    retq
Address range 0x401128 to 0x401134:
   0x0000000000401128 <+-14>:   push   %rbp
   0x0000000000401129 <+-13>:   mov    %rsp,%rbp
   0x000000000040112c <+-10>:   callq  0x401126 <baz>
   0x0000000000401131 <+-5>:    nop
   0x0000000000401132 <+-4>:    pop    %rbp
   0x0000000000401133 <+-3>:    retq
End of assembler dump.

This is an actual dump from the test case that I constructed for
this work.  The ranges are printed in the order encountered in the
debug info. For the above example, note that the second range occupies
lower addresses than the first range.

Functions with contiguous ranges are still printed as follows:

(gdb) disassemble main
Dump of assembler code for function main:
   0x0000000000401151 <+0>:     push   %rbp
   0x0000000000401152 <+1>:     mov    %rsp,%rbp
   0x0000000000401155 <+4>:     callq  0x401136 <foo>
   0x000000000040115a <+9>:     mov    $0x0,%eax
   0x000000000040115f <+14>:    pop    %rbp
   0x0000000000401160 <+15>:    retq
End of assembler dump.

gdb/ChangeLog:
    
    	* cli/cli-cmds.c (block.h): Include.
    	(print_disassembly): Handle printing of non-contiguous blocks.
    	(disassemble_current_function): Likewise.
    	(disassemble_command): Likewise.
---
 gdb/cli/cli-cmds.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5c5d6dc..171936c 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -38,6 +38,7 @@
 #include "tracepoint.h"
 #include "filestuff.h"
 #include "location.h"
+#include "block.h"
 
 #include "ui-out.h"
 
@@ -1096,6 +1097,7 @@ list_command (const char *arg, int from_tty)
 static void
 print_disassembly (struct gdbarch *gdbarch, const char *name,
 		   CORE_ADDR low, CORE_ADDR high,
+		   const struct block *b,
 		   gdb_disassembly_flags flags)
 {
 #if defined(TUI)
@@ -1104,14 +1106,28 @@ print_disassembly (struct gdbarch *gdbarch, const char *name,
     {
       printf_filtered ("Dump of assembler code ");
       if (name != NULL)
-        printf_filtered ("for function %s:\n", name);
-      else
-        printf_filtered ("from %s to %s:\n",
-			 paddress (gdbarch, low), paddress (gdbarch, high));
-
-      /* Dump the specified range.  */
-      gdb_disassembly (gdbarch, current_uiout, flags, -1, low, high);
+	printf_filtered ("for function %s:\n", name);
+      if (!b || BLOCK_CONTIGUOUS_P (b))
+        {
+	  if (name == NULL)
+	    printf_filtered ("from %s to %s:\n",
+			     paddress (gdbarch, low), paddress (gdbarch, high));
 
+	  /* Dump the specified range.  */
+	  gdb_disassembly (gdbarch, current_uiout, flags, -1, low, high);
+	}
+      else
+        {
+	  for (int i = 0; i < BLOCK_NRANGES (b); i++)
+	    {
+	      CORE_ADDR low = BLOCK_RANGE_START (b, i);
+	      CORE_ADDR high = BLOCK_RANGE_END (b, i);
+	      printf_filtered ("Address range %s to %s:\n",
+			       paddress (gdbarch, low),
+			       paddress (gdbarch, high));
+	      gdb_disassembly (gdbarch, current_uiout, flags, -1, low, high);
+	    }
+	}
       printf_filtered ("End of assembler dump.\n");
       gdb_flush (gdb_stdout);
     }
@@ -1133,11 +1149,12 @@ disassemble_current_function (gdb_disassembly_flags flags)
   struct gdbarch *gdbarch;
   CORE_ADDR low, high, pc;
   const char *name;
+  const struct block *b;
 
   frame = get_selected_frame (_("No frame selected."));
   gdbarch = get_frame_arch (frame);
   pc = get_frame_address_in_block (frame);
-  if (find_pc_partial_function (pc, &name, &low, &high) == 0)
+  if (find_pc_partial_function (pc, &name, &low, &high, &b) == 0)
     error (_("No function contains program counter for selected frame."));
 #if defined(TUI)
   /* NOTE: cagney/2003-02-13 The `tui_active' was previously
@@ -1148,7 +1165,7 @@ disassemble_current_function (gdb_disassembly_flags flags)
 #endif
   low += gdbarch_deprecated_function_start_offset (gdbarch);
 
-  print_disassembly (gdbarch, name, low, high, flags);
+  print_disassembly (gdbarch, name, low, high, b, flags);
 }
 
 /* Dump a specified section of assembly code.
@@ -1184,6 +1201,7 @@ disassemble_command (const char *arg, int from_tty)
   CORE_ADDR pc;
   gdb_disassembly_flags flags;
   const char *p;
+  const struct block *b = nullptr;
 
   p = arg;
   name = NULL;
@@ -1234,7 +1252,7 @@ disassemble_command (const char *arg, int from_tty)
   if (p[0] == '\0')
     {
       /* One argument.  */
-      if (find_pc_partial_function (pc, &name, &low, &high) == 0)
+      if (find_pc_partial_function (pc, &name, &low, &high, &b) == 0)
 	error (_("No function contains specified address."));
 #if defined(TUI)
       /* NOTE: cagney/2003-02-13 The `tui_active' was previously
@@ -1262,7 +1280,7 @@ disassemble_command (const char *arg, int from_tty)
 	high += low;
     }
 
-  print_disassembly (gdbarch, name, low, high, flags);
+  print_disassembly (gdbarch, name, low, high, b, flags);
 }
 
 static void

[PATCH 5/8] Use BLOCK_ENTRY_PC in place of most uses of BLOCK_START

[Kevin Buettner]

This change/patch substitues BLOCK_ENTRY_PC for BLOCK_START in
places where BLOCK_START is used to obtain the address at which
execution should enter the block.  Since blocks can now contain
non-contiguous ranges, the BLOCK_START - which is still be the
very lowest address in the block - might not be the same as
BLOCK_ENTRY_PC.

There is a change to infrun.c which is less obvious and less mechanical.
I'm posting it as a separate patch.

gdb/ChangeLog:
    
    	* ax-gdb.c (gen_var_ref): Use BLOCK_ENTRY_PC in place of
    	BLOCK_START.
    	* blockframe.c (get_pc_function_start): Likewise.
    	* compile/compile-c-symbols.c (convert_one_symbol): Likewise.
    	(gcc_symbol_address): Likewise.
    	* compile/compile-object-run.c (compile_object_run): Likewise.
    	* compile/compile.c (get_expr_block_and_pc): Likewise.
    	* dwarf2loc.c (dwarf2_find_location_expression): Likewise.
    	(func_addr_to_tail_call_list): Likewise.
    	* findvar.c (default_read_var_value): Likewise.
    	* inline-frame.c (inline_frame_this_id): Likewise.
    	(skip-inline_frames): Likewise.
    	* infcmd.c (until_next_command): Likewise.
    	* linespec.c (convert_linespec_to_sals): Likewise.
    	* parse.c (parse_exp_in_context_1): Likewise.
    	* printcmd.c (build_address_symbolic): likewise.
    	(info_address_command): Likewise.
    	symtab.c (find_function_start_sal): Likewise.
    	(skip_prologue_sal): Likewise.
    	(find_function_alias_target): Likewise.
    	(find_gnu_ifunc): Likewise.
    	* stack.c (find_frame_funname): Likewise.
    	* symtab.c (fixup_symbol_section): Likewise.
    	(find_function_start_sal): Likewise.
    	(skip_prologue_sal): Likewsie.
    	(find_function_alias_target): Likewise.
    	(find_gnu_ifunc): Likewise.
    	* tracepoint.c (info_scope_command): Likewise.
    	* value.c (value_fn_field): Likewise.
---
 gdb/ax-gdb.c                     |  2 +-
 gdb/blockframe.c                 |  4 ++--
 gdb/compile/compile-c-symbols.c  |  4 ++--
 gdb/compile/compile-object-run.c |  2 +-
 gdb/compile/compile.c            |  4 ++--
 gdb/dwarf2loc.c                  |  4 ++--
 gdb/findvar.c                    |  4 ++--
 gdb/infcmd.c                     |  2 +-
 gdb/inline-frame.c               |  6 +++---
 gdb/linespec.c                   |  2 +-
 gdb/parse.c                      |  4 ++--
 gdb/printcmd.c                   |  4 ++--
 gdb/stack.c                      |  2 +-
 gdb/symtab.c                     | 12 ++++++------
 gdb/tracepoint.c                 |  4 ++--
 gdb/value.c                      |  2 +-
 16 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 2468061..9abd939 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -679,7 +679,7 @@ gen_var_ref (struct agent_expr *ax, struct axs_value *value, struct symbol *var)
       break;
 
     case LOC_BLOCK:
-      ax_const_l (ax, BLOCK_START (SYMBOL_BLOCK_VALUE (var)));
+      ax_const_l (ax, BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (var)));
       value->kind = axs_rvalue;
       break;
 
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 3892c46..0bc2d4d 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -96,7 +96,7 @@ get_pc_function_start (CORE_ADDR pc)
       if (symbol)
 	{
 	  bl = SYMBOL_BLOCK_VALUE (symbol);
-	  return BLOCK_START (bl);
+	  return BLOCK_ENTRY_PC (bl);
 	}
     }
 
@@ -309,7 +309,7 @@ find_function_type (CORE_ADDR pc)
 {
   struct symbol *sym = find_pc_function (pc);
 
-  if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc)
+  if (sym != NULL && BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym)) == pc)
     return SYMBOL_TYPE (sym);
 
   return NULL;
diff --git a/gdb/compile/compile-c-symbols.c b/gdb/compile/compile-c-symbols.c
index 43de7df..936e170 100644
--- a/gdb/compile/compile-c-symbols.c
+++ b/gdb/compile/compile-c-symbols.c
@@ -184,7 +184,7 @@ convert_one_symbol (struct compile_c_instance *context,
 
 	case LOC_BLOCK:
 	  kind = GCC_C_SYMBOL_FUNCTION;
-	  addr = BLOCK_START (SYMBOL_BLOCK_VALUE (sym.symbol));
+	  addr = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym.symbol));
 	  if (is_global && TYPE_GNU_IFUNC (SYMBOL_TYPE (sym.symbol)))
 	    addr = gnu_ifunc_resolve_addr (target_gdbarch (), addr);
 	  break;
@@ -497,7 +497,7 @@ gcc_symbol_address (void *datum, struct gcc_c_context *gcc_context,
 	    fprintf_unfiltered (gdb_stdlog,
 				"gcc_symbol_address \"%s\": full symbol\n",
 				identifier);
-	  result = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+	  result = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym));
 	  if (TYPE_GNU_IFUNC (SYMBOL_TYPE (sym)))
 	    result = gnu_ifunc_resolve_addr (target_gdbarch (), result);
 	  found = 1;
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index e8a95e3..0846c73 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -152,7 +152,7 @@ compile_object_run (struct compile_module *module)
 
       gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC);
       func_val = value_from_pointer (lookup_pointer_type (func_type),
-				   BLOCK_START (SYMBOL_BLOCK_VALUE (func_sym)));
+				   BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func_sym)));
 
       vargs = XALLOCAVEC (struct value *, TYPE_NFIELDS (func_type));
       if (TYPE_NFIELDS (func_type) >= 1)
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 01c0bc4..be1cca0 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -272,10 +272,10 @@ get_expr_block_and_pc (CORE_ADDR *pc)
 	block = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (cursal.symtab),
 				   STATIC_BLOCK);
       if (block != NULL)
-	*pc = BLOCK_START (block);
+	*pc = BLOCK_ENTRY_PC (block);
     }
   else
-    *pc = BLOCK_START (block);
+    *pc = BLOCK_ENTRY_PC (block);
 
   return block;
 }
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 730934f..2da088e 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -361,7 +361,7 @@ dwarf2_find_location_expression (struct dwarf2_loclist_baton *baton,
 	  if (pc_block)
 	    pc_func = block_linkage_function (pc_block);
 
-	  if (pc_func && pc == BLOCK_START (SYMBOL_BLOCK_VALUE (pc_func)))
+	  if (pc_func && pc == BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (pc_func)))
 	    {
 	      *locexpr_length = length;
 	      return loc_ptr;
@@ -833,7 +833,7 @@ func_addr_to_tail_call_list (struct gdbarch *gdbarch, CORE_ADDR addr)
   struct symbol *sym = find_pc_function (addr);
   struct type *type;
 
-  if (sym == NULL || BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) != addr)
+  if (sym == NULL || BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym)) != addr)
     throw_error (NO_ENTRY_VALUE_ERROR,
 		 _("DW_TAG_call_site resolving failed to find function "
 		   "name for address %s"),
diff --git a/gdb/findvar.c b/gdb/findvar.c
index 8ad5e25..09e5c29 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -702,10 +702,10 @@ default_read_var_value (struct symbol *var, const struct block *var_block,
     case LOC_BLOCK:
       if (overlay_debugging)
 	addr = symbol_overlayed_address
-	  (BLOCK_START (SYMBOL_BLOCK_VALUE (var)),
+	  (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (var)),
 	   SYMBOL_OBJ_SECTION (symbol_objfile (var), var));
       else
-	addr = BLOCK_START (SYMBOL_BLOCK_VALUE (var));
+	addr = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (var));
       break;
 
     case LOC_REGISTER:
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5c5faf7..2b17957 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1554,7 +1554,7 @@ until_next_command (int from_tty)
     {
       sal = find_pc_line (pc, 0);
 
-      tp->control.step_range_start = BLOCK_START (SYMBOL_BLOCK_VALUE (func));
+      tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
       tp->control.step_range_end = sal.end;
     }
   tp->control.may_range_step = 1;
diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 3edd5b2..53830fd 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -165,7 +165,7 @@ inline_frame_this_id (struct frame_info *this_frame,
      in the frame ID (and eventually, to set breakpoints).  */
   func = get_frame_function (this_frame);
   gdb_assert (func != NULL);
-  (*this_id).code_addr = BLOCK_START (SYMBOL_BLOCK_VALUE (func));
+  (*this_id).code_addr = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
   (*this_id).artificial_depth++;
 }
 
@@ -333,8 +333,8 @@ skip_inline_frames (ptid_t ptid, bpstat stop_chain)
 	  if (block_inlined_p (cur_block))
 	    {
 	      /* See comments in inline_frame_this_id about this use
-		 of BLOCK_START.  */
-	      if (BLOCK_START (cur_block) == this_pc
+		 of BLOCK_ENTRY_PC.  */
+	      if (BLOCK_ENTRY_PC (cur_block) == this_pc
 		  || block_starting_point_at (this_pc, cur_block))
 		{
 		  /* Do not skip the inlined frame if execution
diff --git a/gdb/linespec.c b/gdb/linespec.c
index ae0200b..1736e59 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2283,7 +2283,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 		   && SYMBOL_CLASS (sym) == LOC_BLOCK)
 		{
 		  const CORE_ADDR addr
-		    = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+		    = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym));
 
 		  bound_minimal_symbol_d *elem;
 		  for (int m = 0;
diff --git a/gdb/parse.c b/gdb/parse.c
index acb08f2..cd08879 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -1147,7 +1147,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
   if (!expression_context_block)
     expression_context_block = get_selected_block (&expression_context_pc);
   else if (pc == 0)
-    expression_context_pc = BLOCK_START (expression_context_block);
+    expression_context_pc = BLOCK_ENTRY_PC (expression_context_block);
   else
     expression_context_pc = pc;
 
@@ -1161,7 +1161,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
 	  = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (cursal.symtab),
 			       STATIC_BLOCK);
       if (expression_context_block)
-	expression_context_pc = BLOCK_START (expression_context_block);
+	expression_context_pc = BLOCK_ENTRY_PC (expression_context_block);
     }
 
   if (language_mode == language_mode_auto && block != NULL)
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 696285e..3c6e4be 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -609,7 +609,7 @@ build_address_symbolic (struct gdbarch *gdbarch,
 	 pointer is <function+3>.  This matches the ISA behavior.  */
       addr = gdbarch_addr_bits_remove (gdbarch, addr);
 
-      name_location = BLOCK_START (SYMBOL_BLOCK_VALUE (symbol));
+      name_location = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (symbol));
       if (do_demangle || asm_demangle)
 	name_temp = SYMBOL_PRINT_NAME (symbol);
       else
@@ -1520,7 +1520,7 @@ info_address_command (const char *exp, int from_tty)
 
     case LOC_BLOCK:
       printf_filtered (_("a function at address "));
-      load_addr = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+      load_addr = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym));
       fputs_filtered (paddress (gdbarch, load_addr), gdb_stdout);
       if (section_is_overlay (section))
 	{
diff --git a/gdb/stack.c b/gdb/stack.c
index 97ebc8b..5d3110d 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1079,7 +1079,7 @@ find_frame_funname (struct frame_info *frame, enum language *funlang,
 
       if (msymbol.minsym != NULL
 	  && (BMSYMBOL_VALUE_ADDRESS (msymbol)
-	      > BLOCK_START (SYMBOL_BLOCK_VALUE (func))))
+	      > BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func))))
 	{
 	  /* We also don't know anything about the function besides
 	     its address and name.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 3e594e7..4eb8a4f 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1746,7 +1746,7 @@ fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
       addr = SYMBOL_VALUE_ADDRESS (sym);
       break;
     case LOC_BLOCK:
-      addr = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+      addr = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym));
       break;
 
     default:
@@ -3620,7 +3620,7 @@ find_function_start_sal (symbol *sym, bool funfirstline)
 {
   fixup_symbol_section (sym, NULL);
   symtab_and_line sal
-    = find_function_start_sal (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)),
+    = find_function_start_sal (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym)),
 			       SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym),
 			       funfirstline);
   sal.symbol = sym;
@@ -3700,7 +3700,7 @@ skip_prologue_sal (struct symtab_and_line *sal)
       fixup_symbol_section (sym, NULL);
 
       objfile = symbol_objfile (sym);
-      pc = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+      pc = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym));
       section = SYMBOL_OBJ_SECTION (objfile, sym);
       name = SYMBOL_LINKAGE_NAME (sym);
     }
@@ -3761,7 +3761,7 @@ skip_prologue_sal (struct symtab_and_line *sal)
       /* Check if gdbarch_skip_prologue left us in mid-line, and the next
 	 line is still part of the same function.  */
       if (skip && start_sal.pc != pc
-	  && (sym ? (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) <= start_sal.end
+	  && (sym ? (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym)) <= start_sal.end
 		     && start_sal.end < BLOCK_END (SYMBOL_BLOCK_VALUE (sym)))
 	      : (lookup_minimal_symbol_by_pc_section (start_sal.end, section).minsym
 		 == lookup_minimal_symbol_by_pc_section (pc, section).minsym)))
@@ -3965,7 +3965,7 @@ find_function_alias_target (bound_minimal_symbol msymbol)
   symbol *sym = find_pc_function (func_addr);
   if (sym != NULL
       && SYMBOL_CLASS (sym) == LOC_BLOCK
-      && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == func_addr)
+      && BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym)) == func_addr)
     return sym;
 
   return NULL;
@@ -4970,7 +4970,7 @@ find_gnu_ifunc (const symbol *sym)
 				symbol_name_match_type::SEARCH_NAME);
   struct objfile *objfile = symbol_objfile (sym);
 
-  CORE_ADDR address = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+  CORE_ADDR address = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym));
   minimal_symbol *ifunc = NULL;
 
   iterate_over_minimal_symbols (objfile, lookup_name,
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 5af3cfe..a1a3524 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2519,7 +2519,7 @@ info_scope_command (const char *args_in, int from_tty)
 
 	  if (SYMBOL_COMPUTED_OPS (sym) != NULL)
 	    SYMBOL_COMPUTED_OPS (sym)->describe_location (sym,
-							  BLOCK_START (block),
+							  BLOCK_ENTRY_PC (block),
 							  gdb_stdout);
 	  else
 	    {
@@ -2596,7 +2596,7 @@ info_scope_command (const char *args_in, int from_tty)
 		case LOC_BLOCK:
 		  printf_filtered ("a function at address ");
 		  printf_filtered ("%s",
-				   paddress (gdbarch, BLOCK_START (SYMBOL_BLOCK_VALUE (sym))));
+				   paddress (gdbarch, BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym))));
 		  break;
 		case LOC_UNRESOLVED:
 		  msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (sym),
diff --git a/gdb/value.c b/gdb/value.c
index 9f9e78e..7c5c79e 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3051,7 +3051,7 @@ value_fn_field (struct value **arg1p, struct fn_field *f,
   VALUE_LVAL (v) = lval_memory;
   if (sym)
     {
-      set_value_address (v, BLOCK_START (SYMBOL_BLOCK_VALUE (sym)));
+      set_value_address (v, BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (sym)));
     }
   else
     {

[PATCH 6/8] Use BLOCK_ENTRY_PC to find function entry pc in infrun.c

[Kevin Buettner]

find_pc_partial_function() still returns the lowest and highest address
of a function even when that function contains non-contiguous ranges.
In cases where greater discrimination is required, the BLOCK parameter
may be examined to determine the actual entry PC.

This patch uses the BLOCK return value from find_pc_partial_function()
to obtain the entry PC. If no block is found - which can happen when
only minimal symbols are available - then the start address provided
by find_pc_partial_function() will still be used (as before).

gdb/ChangeLog:
    
    	* infrun.c (fill_in_stop_func): Use BLOCK_ENTRY_PC to
    	determine stop_func_start in the execution control state.
---
 gdb/infrun.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index f455af2..88d8fdc 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4297,10 +4297,20 @@ fill_in_stop_func (struct gdbarch *gdbarch,
 {
   if (!ecs->stop_func_filled_in)
     {
+      const struct block *block;
+
       /* Don't care about return value; stop_func_start and stop_func_name
 	 will both be 0 if it doesn't work.  */
       find_pc_partial_function (stop_pc, &ecs->stop_func_name,
-				&ecs->stop_func_start, &ecs->stop_func_end);
+				&ecs->stop_func_start, &ecs->stop_func_end,
+				&block);
+
+      /* If a block is returned, prefer the block's entry point instead of
+         the lowest address of the block - these aren't necessarily the
+	 same.  */
+      if (block)
+	ecs->stop_func_start = BLOCK_ENTRY_PC (block);
+
       ecs->stop_func_start
 	+= gdbarch_deprecated_function_start_offset (gdbarch);
 

[PATCH 7/8] Relocate block range start and end addresses

[Kevin Buettner]

gdb/ChangeLog:
    
    	* objfiles.c (objfile_relocate1): Relocate start and end addresses
    	for each range in a block.
---
 gdb/objfiles.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 95c39cf..21ca111 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -831,6 +831,14 @@ objfile_relocate1 (struct objfile *objfile,
 	  BLOCK_START (b) += ANOFFSET (delta, block_line_section);
 	  BLOCK_END (b) += ANOFFSET (delta, block_line_section);
 
+	  if (BLOCK_RANGES (b))
+	    for (int j = 0; j < BLOCK_NRANGES (b); j++)
+	      {
+		BLOCK_RANGE_START (b, j)
+		  += ANOFFSET (delta, block_line_section);
+		BLOCK_RANGE_END (b, j) += ANOFFSET (delta, block_line_section);
+	      }
+
 	  /* We only want to iterate over the local symbols, not any
 	     symbols in included symtabs.  */
 	  ALL_DICT_SYMBOLS (BLOCK_DICT (b), iter, sym)

[PATCH 8/8] Test case for functions with non-contiguous ranges

[Kevin Buettner]

See comments in the new files for what this is about - I tried to
explain it all there.

gdb/testsuite/ChangeLog:
    
    	* gdb.dwarf2/dw2-ranges-func.c: New file.
    	* gdb.dwarf2/dw2-ranges-func.exp: New file.
---
 gdb/testsuite/gdb.dwarf2/dw2-ranges-func.c   |  78 +++++
 gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp | 410 +++++++++++++++++++++++++++
 2 files changed, 488 insertions(+)

diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.c b/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.c
new file mode 100644
index 0000000..864803c
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.c
@@ -0,0 +1,78 @@
+/* Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* The idea here is to, via use of the dwarf assembler, create a function
+   which occupies two non-contiguous address ranges.
+
+   foo_low and foo will be combined into a single function foo with a
+   function bar in between these two ranges.
+
+   This test case was motivated by a bug in which a function which
+   occupied two non-contiguous address ranges was calling another
+   function which resides in between these ranges.  So we end up with
+   a situation in which the low/start address of our constructed foo
+   (in this case) will be less than any of the addresses in bar, but
+   the high/end address of foo will be greater than any of bar's
+   addresses.
+
+   This situation was causing a problem in the caching code of
+   find_pc_partial_function:  When the low and high addresses of foo
+   are placed in the cache, the simple check that was used to see if
+   the cache was applicable would (incorrectly) succeed when presented
+   with an address in bar.  I.e. an address in bar presented as an
+   input to find_pc_partial_function could produce the answer "this
+   address belongs to foo".  */
+
+volatile int e = 0;
+
+void
+baz (void)
+{
+  asm ("baz_label: .globl baz_label");
+}						/* baz end */
+
+void
+foo_low (void)
+{						/* foo_low prologue */
+  asm ("foo_low_label: .globl foo_low_label");
+  baz ();					/* foo_low baz call */
+  asm ("foo_low_label2: .globl foo_low_label2");
+}						/* foo_low end */
+
+void
+bar (void)
+{
+  asm ("bar_label: .globl bar_label");
+}						/* bar end */
+
+void
+foo (void)
+{						/* foo prologue */
+  asm ("foo_label: .globl foo_label");
+  bar ();					/* foo bar call */
+  asm ("foo_label2: .globl foo_label2");
+  if (e) foo_low ();				/* foo foo_low call */
+  asm ("foo_label3: .globl foo_label3");
+}						/* foo end */
+
+int
+main (void)
+{						/* main prologue */
+  asm ("main_label: .globl main_label");
+  foo ();					/* main foo call */
+  asm ("main_label2: .globl main_label2");
+  return 0;					/* main return */
+}						/* main end */
+
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp b/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp
new file mode 100644
index 0000000..6adb471
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp
@@ -0,0 +1,410 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# Test DW_AT_ranges in the context of a subprogram scope.
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    verbose "Skipping DW_AT_ranges test."
+    return 0
+}
+
+# The .c files use __attribute__.
+if [get_compiler_info] {
+    return -1
+}
+if !$gcc_compiled {
+    verbose "Skipping DW_AT_ranges test."
+    return 0
+}
+
+standard_testfile dw2-ranges-func.c dw2-ranges-func-dw.S
+
+# We need to know the size of integer and address types in order to
+# write some of the debugging info we'd like to generate.
+#
+# For that, we ask GDB by debugging our test program.  Any program
+# would do, but since we already have it specifically for this
+# testcase, might as well use that.
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile2
+    declare_labels integer_label volatile_label ranges_label ranges2_label L
+    set int_size [get_sizeof "int" 4]
+
+    # Find start address and length for our functions.
+    set main_func \
+	[function_range main [list ${srcdir}/${subdir}/$srcfile]]
+    set foo_func \
+	[function_range foo [list ${srcdir}/${subdir}/$srcfile]]
+    set foo_low_func \
+	[function_range foo_low [list ${srcdir}/${subdir}/$srcfile]]
+    set bar_func \
+	[function_range bar [list ${srcdir}/${subdir}/$srcfile]]
+    set baz_func \
+	[function_range baz [list ${srcdir}/${subdir}/$srcfile]]
+
+    set main_start [lindex $main_func 0]
+    set main_len [lindex $main_func 1]
+    set foo_start [lindex $foo_func 0]
+    set foo_end {$foo_start + [lindex $foo_func 1]}
+    set foo_low_start [lindex $foo_low_func 0]
+    set foo_low_len [lindex $foo_low_func 1]
+    set foo_low_end {$foo_low_start + $foo_low_len}
+    set bar_start [lindex $bar_func 0]
+    set bar_len [lindex $bar_func 1]
+    set baz_start [lindex $baz_func 0]
+    set baz_len [lindex $baz_func 1]
+
+    set e_var [gdb_target_symbol e]
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name dw-ranges-func.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+	    {low_pc 0 addr}
+	    {ranges ${ranges2_label} DW_FORM_sec_offset}
+	} {
+            integer_label: DW_TAG_base_type {
+                {DW_AT_byte_size $int_size DW_FORM_sdata}
+                {DW_AT_encoding  @DW_ATE_signed}
+                {DW_AT_name      integer}
+            }
+	    volatile_label: DW_TAG_volatile_type {
+		{type :$integer_label}
+	    }
+	    DW_TAG_variable {
+		{name e}
+		{external 1 flag}
+		{type :$volatile_label}
+		{location {addr $e_var} SPECIAL_expr}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{name main}
+		{DW_AT_type :$integer_label}
+		{low_pc $main_start addr}
+		{high_pc $main_len DW_FORM_data4}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{name foo}
+		{ranges ${ranges_label} DW_FORM_sec_offset}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{name bar}
+		{low_pc $bar_start addr}
+		{high_pc $bar_len DW_FORM_data4}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{name baz}
+		{low_pc $baz_start addr}
+		{high_pc $baz_len DW_FORM_data4}
+	    }
+	}
+    }
+
+    lines {version 2} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	# Generate a line table program. I attempted to make it reasonably
+	# accurate as it made debugging the test case easier.
+	program {
+	    {DW_LNE_set_address [lindex $main_func 0]}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "main prologue"] - 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "main foo call"] - [gdb_get_line_number "main prologue"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_label2}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "main return"] - [gdb_get_line_number "main foo call"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address [lindex $main_func 0]+[lindex $main_func 1]}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "main end"] - [gdb_get_line_number "main return"] + 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address [lindex $foo_func 0]}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo prologue"] - 1] }
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_label}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo bar call"] - [gdb_get_line_number "foo prologue"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_label2}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo foo_low call"] - [gdb_get_line_number "foo bar call"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_label3}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo end"] - [gdb_get_line_number "foo foo_low call"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address [lindex $foo_func 0]+[lindex $foo_func 1]}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address [lindex $bar_func 0]}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "bar end"] - 1]}
+	    {DW_LNS_copy}
+	    {DW_LNS_advance_pc [lindex $bar_func 1]}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address [lindex $baz_func 0]}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "baz end"] - 1]}
+	    {DW_LNS_copy}
+	    {DW_LNS_advance_pc [lindex $baz_func 1]}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+
+	    {DW_LNE_set_address [lindex $foo_low_func 0]}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo_low prologue"] - 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_low_label}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo_low baz call"] - [gdb_get_line_number "foo_low prologue"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address foo_low_label2}
+	    {DW_LNS_advance_line [expr [gdb_get_line_number "foo_low end"] - [gdb_get_line_number "foo_low baz call"]]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address [lindex $foo_low_func 0]+[lindex $foo_low_func 1]}
+	    {DW_LNS_advance_line 1}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+	}
+    }
+
+    # Generate ranges data.
+    ranges {is_64 [is_64_target]} {
+	ranges_label: sequence {
+	    {range {$foo_start } $foo_end}
+	    {range {$foo_low_start} $foo_low_end}
+	}
+	ranges2_label: sequence {
+	    {range {$foo_start } $foo_end}
+	    {range {$foo_low_start} $foo_low_end}
+	    {range {$main_start} $main_start + $main_len}
+	    {range {$bar_start} $bar_start + $bar_len}
+	    {range {$baz_start} $baz_start + $baz_len}
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+set main_prologue_line_num [gdb_get_line_number "main prologue"]
+# Do a sanity check to make sure that line number info is available.
+gdb_test "info line main" \
+    "Line ${main_prologue_line_num} of .* starts at address .* and ends at .*"
+
+set bp_foo_bar [gdb_get_line_number "foo bar call"]
+
+gdb_test "break $bp_foo_bar" \
+    "Breakpoint.*at.* file .*$srcfile, line $bp_foo_bar\\." \
+    "break at call to bar"
+
+gdb_test "continue" \
+    "Continuing\\..*Breakpoint \[0-9\]+, foo \\(\\).*$bp_foo_bar\\s+bar\\s\\(\\);.*foo bar call.*" \
+    "continue to call of bar"
+
+gdb_test "step" \
+    "bar \\(\\).*bar end.*" \
+    "step into bar"
+
+gdb_test "step" \
+    "foo \\(\\).*foo foo_low call.*" \
+    "step out of bar, back into foo"
+
+delete_breakpoints
+if ![runto_main] {
+    return -1
+}
+
+# Note that the RE used for the following test will fail when the
+# breakpoint has been set on multiple locations. E.g. "(2 locations)". 
+# This is intentional since that behavior is one of the bugs that
+# this test case tests for.
+gdb_test "break foo" \
+    "Breakpoint.*at.* file .*$srcfile, line \\d+\\." \
+    "break foo"
+
+# Continue to foo.  Allow execution to stop either on the prologue
+# or on the call to bar since either behavior is acceptable though
+# the latter is preferred.
+set test "continue to foo"
+gdb_test_multiple "continue" $test {
+    -re "Breakpoint \\d+, foo \\(\\).*foo prologue.*${gdb_prompt}" {
+        pass $test
+	gdb_test "step" \
+	         "foo bar call .*" \
+		 "step to call of bar after landing on prologue"
+    }
+    -re "Breakpoint \\d+, foo \\(\\).*foo bar call.*${gdb_prompt}" {
+        pass $test
+    }
+}
+
+gdb_test "step" \
+    "bar \\(\\).*bar end.*" \
+    "step into bar (2nd time)"
+
+gdb_test "step" \
+    "foo \\(\\).*foo foo_low call.*" \
+    "step out of bar, back into foo (2nd time)"
+
+delete_breakpoints
+if ![runto_main] {
+    return -1
+}
+
+# Disassembly of foo should have multiple address ranges.
+# Note: I couldn't get $hex to work in the gdb_test_sequence regex.
+gdb_test_sequence "disassemble foo" "" {
+    "Dump of assembler code for function foo:"
+    "Address range 0x[\\dabcdefABCDEF]+ to 0x[\\dabcdefABCDEF]+:"
+    "   0x[\\dabcdefABCDEF]+ <\\+0>:"
+    "Address range 0x[\\dabcdefABCDEF]+ to 0x[\\dabcdefABCDEF]+:"
+    "   0x[\\dabcdefABCDEF]+ <(.+?)>:"
+    "End of assembler dump\\." 
+}
+
+set test "x/i foo_low"
+gdb_test_multiple $test $test {
+    -re "   ($hex) <foo.*?>.*${gdb_prompt}" {
+        set foo_low_addr $expect_out(1,string)
+	pass $test
+    }
+}
+
+set test "x/i foo"
+gdb_test_multiple $test $test {
+    -re "   ($hex) <foo.*?>.*${gdb_prompt}" {
+        set foo_addr $expect_out(1,string)
+	pass $test
+    }
+}
+
+set test "foo and foo_low are at different addresses"
+if {$foo_low_addr == $foo_addr} {
+    fail $test
+} else {
+    pass $test
+}
+
+# This more permissive RE for "break foo" will allow a breakpoint on
+# multiple locations to PASS.  */
+gdb_test "break foo" \
+    "Breakpoint.*at.*" \
+    "break foo (2nd time)"
+
+gdb_test "break baz" \
+    "Breakpoint.*at.* file .*$srcfile, line \\d+\\."
+
+gdb_test "continue" \
+    "Breakpoint \\d+, foo \\(\\).*" \
+    "Continue to foo (2nd time)"
+
+gdb_test_no_output "set variable e=1"
+
+# If GDB incorrectly places the foo breakpoint on multiple locations,
+# then GDB will (incorrectly) stop in foo_low instead of in baz.
+gdb_test "continue" \
+    "Breakpoint \\d+, (?:$hex in )?baz \\(\\).*" \
+    "Continue to baz"
+
+delete_breakpoints
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "step" \
+         "foo \\(\\).*bar \\(\\);.*foo bar call.*" \
+	 "step into foo from main"
+
+gdb_test "step" \
+         "bar \\(\\).*}.* bar end.*" \
+	 "step into bar from foo"
+
+gdb_test "step" \
+         "foo(_label2)? \\(\\).*foo_low \\(\\);.*foo foo_low call.*" \
+	 "step out of bar to foo"
+
+# The tests in the "enable_foo_low_stepping" work with some versions
+# of gcc, though it's not clear to me that they should.  This example,
+# which causes foo_low to be considered as part of foo via use of
+# DW_AT_ranges.  Real code that I've looked at uses a branch instruction
+# to cause code in the "cold" range to be executed.  If a subroutine
+# call were used, it would be considered to be a separate subprogram
+# and the issues that I see wouldn't be encountered.
+#
+# For the moment though, I've left these tests in place, but disabled,
+# in the event that we decide that making such a subroutine call is
+# a reasonable thing to do that should also be supported by GDB.
+
+set enable_foo_low_stepping false
+
+if { $enable_foo_low_stepping } {
+    gdb_test_no_output "set variable e=1"
+
+    set test "step into foo_low from foo"
+    gdb_test_multiple "step" $test {
+	-re "foo(_low)? \\(\\).*\{.*foo_low prologue.*${gdb_prompt}" {
+	    pass $test
+	    gdb_test "step" \
+		     "foo \\(\\).*baz \\(\\);.*foo_low baz call.*" \
+		     "step to baz call in foo_low"
+
+	}
+	-re "foo(_low)? \\(\\).*baz \\(\\);.*foo_low baz call.*${gdb_prompt}" {
+	    pass $test
+	}
+    }
+
+    gdb_test "step" \
+	     "baz \\(\\).*\}.*baz end.*" \
+	     "step into baz from foo_low"
+
+    gdb_test "step" \
+	     "foo(?:_low(?:_label2)?)? \\(\\).*\}.*foo_low end.*" \
+	     "step out of baz to foo_low"
+
+    gdb_test "step" \
+	     "foo(?:_label3)? \\(\\).*\}.*foo end.*" \
+	     "step out of foo_low to foo"
+} else {
+    gdb_test "next" \
+	     ".*foo end.*" \
+	     "next over foo_low call"
+}
+
+gdb_test "step" \
+         "main(?:_label2)? \\(\\).*" \
+	 "step out of foo to main"

Re: [PATCH 0/8] Non-contiguous address range support

[Kevin Buettner]

Ping.

On Mon, 25 Jun 2018 23:32:39 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> This eight part patch sequence adds (further) support for
> non-contiguous address ranges to GDB.
> 
> This sequence of patches was motivated by GCC bug 84550:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84550
> 
> There is a test case posted to that bug along with some analysis of
> the underlying problem.
> 
> There is also a GDB bug for the same issue; it's 23021, but at the
> moment, there is little there aside from a link to the GCC bug
> mentioned above.  But here's a link anyway:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=23021
> 
> A quick synopsis of the problem is as follows...
> 
> Recent versions of gcc can generate code in which a function is split
> into at least two non-contiguous address ranges.  As I understand it,
> the idea here is to separate code which gcc does not expect to execute
> in normal operation from the rest of the code.  Doing this may result
> in better cache locality for the normal case.  The generated code for
> the example in GCC bug 84550 separated a call to abort() from the rest
> of the code comprising the function.
> 
> In the course of my investigation, I identified at least four
> problems:
> 
> 1) Stepping into a function from a function which occupies non-contiguous
>    address ranges does not always work.  It was not uncommon to see the
>    program run to completion when attempting to do a step.
> 
> 2) Setting a breakpoint on a function with non-contiguous address ranges
>    causes a breakpoint to be placed on more than one location.  When a
>    breakpoint is set on the "cold" address range, this is almost certainly
>    incorrect.  The breakpoint should instead be set only on code near the
>    entry point(s).
> 
> 3) The disassemble command did not work correctly.  E.g. here is what I
>    found during my analysis of 84550:
> 
> 	(gdb) x/i 'main.cold.0'
> 	   0x4010e0 <main()>:   mov    %rax,%rdi
> 	(gdb) x/i main
> 	   0x4011a0 <main>:     push   %r12
> 	(gdb) disassemble main
> 	Dump of assembler code for function main():
> 	   0x00000000004010e0 <+0>:     mov    %rax,%rdi
> 	   ...
>         [No addresses starting at 0x4011a0 are shown]
> 
> 4) Display of addresses associated with the non-contiguous function are
>    confusing.  E.g. in the above example, note that GDB thinks that
>    the address associated with main.cold.0 is <main()>, but that there's
>    also a minsym called main which is displayed as <main>.
> 
> There are probably several other problems which are related those
> identified above.
> 
> I discovered that the stepping problem could be "fixed" by disabling
> the find_pc_partial_function cache.  This cache keeps track of the
> most recent result (of calling find_pc_partial_function).  If
> find_pc_partial_function is called with an address which falls within
> the cache range, then that's considered to be a cache hit and the most
> recent result is returned.  Obviously, this won't work correctly for
> functions which occupy non-contiguous (disjoint) address ranges where
> other functions might be placed in the gap.
> 
> So one of the problems that needed to be solved was to make the
> caching code work correctly.  It is interesting to note that stepping
> _did_ work when the cache was disabled.  This is/was due to GDB
> already having some (albeit incomplete) support for non-contiguous
> addresses in the form of blockvector address maps.  Code responsible
> for mapping addresses to blocks (which form the lower levels of
> find_pc_partial_function) handle this case correctly.
> 
> To solve the problem of incorrect disassembly, we need to be able
> to iterate over all of the ranges associated with a block.
> 
> Finally, we need to distinguish between the entry pc and the lowest
> address in a block.  I discovered that this lack of distinction was
> the cause of the remainder of the bugs including some which seemed to
> be introduced by fixing the problems noted above.  Once this
> distinction is made, it will be straightforward to add full support for
> DW_AT_entry_pc.  I considered adding this support as part of this
> patch series, but decided to wait until the community weighs in on my
> work thus far...

Re: [PATCH 0/8] Non-contiguous address range support

[Keith Seitz]

On 07/11/2018 08:27 AM, Kevin Buettner wrote:
> Ping.
> 

FWIW, I've started to look at this a bit this week. Not sure exactly what I'd be able to offer (you are a global maintainer and IANAM afterall)...

Keith

Re: [PATCH 0/8] Non-contiguous address range support

[Simon Marchi]

On 2018-06-26 02:32 AM, Kevin Buettner wrote:
> This eight part patch sequence adds (further) support for
> non-contiguous address ranges to GDB.
> 
> This sequence of patches was motivated by GCC bug 84550:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84550
> 
> There is a test case posted to that bug along with some analysis of
> the underlying problem.
> 
> There is also a GDB bug for the same issue; it's 23021, but at the
> moment, there is little there aside from a link to the GCC bug
> mentioned above.  But here's a link anyway:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=23021
> 
> A quick synopsis of the problem is as follows...
> 
> Recent versions of gcc can generate code in which a function is split
> into at least two non-contiguous address ranges.  As I understand it,
> the idea here is to separate code which gcc does not expect to execute
> in normal operation from the rest of the code.  Doing this may result
> in better cache locality for the normal case.  The generated code for
> the example in GCC bug 84550 separated a call to abort() from the rest
> of the code comprising the function.
> 
> In the course of my investigation, I identified at least four
> problems:
> 
> 1) Stepping into a function from a function which occupies non-contiguous
>    address ranges does not always work.  It was not uncommon to see the
>    program run to completion when attempting to do a step.
> 
> 2) Setting a breakpoint on a function with non-contiguous address ranges
>    causes a breakpoint to be placed on more than one location.  When a
>    breakpoint is set on the "cold" address range, this is almost certainly
>    incorrect.  The breakpoint should instead be set only on code near the
>    entry point(s).
> 
> 3) The disassemble command did not work correctly.  E.g. here is what I
>    found during my analysis of 84550:
> 
> 	(gdb) x/i 'main.cold.0'
> 	   0x4010e0 <main()>:   mov    %rax,%rdi
> 	(gdb) x/i main
> 	   0x4011a0 <main>:     push   %r12
> 	(gdb) disassemble main
> 	Dump of assembler code for function main():
> 	   0x00000000004010e0 <+0>:     mov    %rax,%rdi
> 	   ...
>         [No addresses starting at 0x4011a0 are shown]
> 
> 4) Display of addresses associated with the non-contiguous function are
>    confusing.  E.g. in the above example, note that GDB thinks that
>    the address associated with main.cold.0 is <main()>, but that there's
>    also a minsym called main which is displayed as <main>.
> 
> There are probably several other problems which are related those
> identified above.
> 
> I discovered that the stepping problem could be "fixed" by disabling
> the find_pc_partial_function cache.  This cache keeps track of the
> most recent result (of calling find_pc_partial_function).  If
> find_pc_partial_function is called with an address which falls within
> the cache range, then that's considered to be a cache hit and the most
> recent result is returned.  Obviously, this won't work correctly for
> functions which occupy non-contiguous (disjoint) address ranges where
> other functions might be placed in the gap.
> 
> So one of the problems that needed to be solved was to make the
> caching code work correctly.  It is interesting to note that stepping
> _did_ work when the cache was disabled.  This is/was due to GDB
> already having some (albeit incomplete) support for non-contiguous
> addresses in the form of blockvector address maps.  Code responsible
> for mapping addresses to blocks (which form the lower levels of
> find_pc_partial_function) handle this case correctly.
> 
> To solve the problem of incorrect disassembly, we need to be able
> to iterate over all of the ranges associated with a block.
> 
> Finally, we need to distinguish between the entry pc and the lowest
> address in a block.  I discovered that this lack of distinction was
> the cause of the remainder of the bugs including some which seemed to
> be introduced by fixing the problems noted above.  Once this
> distinction is made, it will be straightforward to add full support for
> DW_AT_entry_pc.  I considered adding this support as part of this
> patch series, but decided to wait until the community weighs in on my
> work thus far...
> 

Hi Kevin,

I just started looking into this, but I already have a question, just for
the sake of the discussion.  After reading this description and the first
few patches, it seems to me like one "cheap" way to fix the caching issue
would be to record the contiguous portion of the address range instead
of the low/high of the complete block.

So if we have this:

  [0x100,0x110) function_a
  [0x110,0x120) function_b
  [0x120,0x130) function_a.cold

When probing for pc = 0x104, we currently save:

  cache_pc_function_low = 0x100
  cache_pc_function_high = 0x130

But instead we would save;

  cache_pc_function_low = 0x100
  cache_pc_function_high = 0x110

I assume that the usage pattern of find_pc_partial_function is querying
multiple times the same pc, or pc in increasing order (e.g. when
disassembling).  This modification wouldn't change the efficiency of
the cache in these situations.

Your solution of recording address ranges for blocks is more complete
and probably necessary for other use cases, I am not questioning that,
but I wanted to know if you had first tried a simpler solution similar
to the above.

Simon

Re: [PATCH 0/8] Non-contiguous address range support

[Kevin Buettner]

On Thu, 12 Jul 2018 15:12:15 -0400
Simon Marchi <simon.marchi@ericsson.com> wrote:

> On 2018-06-26 02:32 AM, Kevin Buettner wrote:
> > This eight part patch sequence adds (further) support for
> > non-contiguous address ranges to GDB.
> > 
> > This sequence of patches was motivated by GCC bug 84550:
> > 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84550
> > 
> > There is a test case posted to that bug along with some analysis of
> > the underlying problem.
> > 
> > There is also a GDB bug for the same issue; it's 23021, but at the
> > moment, there is little there aside from a link to the GCC bug
> > mentioned above.  But here's a link anyway:
> > 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=23021
> > 
> > A quick synopsis of the problem is as follows...
> > 
> > Recent versions of gcc can generate code in which a function is split
> > into at least two non-contiguous address ranges.  As I understand it,
> > the idea here is to separate code which gcc does not expect to execute
> > in normal operation from the rest of the code.  Doing this may result
> > in better cache locality for the normal case.  The generated code for
> > the example in GCC bug 84550 separated a call to abort() from the rest
> > of the code comprising the function.
> > 
> > In the course of my investigation, I identified at least four
> > problems:
> > 
> > 1) Stepping into a function from a function which occupies non-contiguous
> >    address ranges does not always work.  It was not uncommon to see the
> >    program run to completion when attempting to do a step.
> > 
> > 2) Setting a breakpoint on a function with non-contiguous address ranges
> >    causes a breakpoint to be placed on more than one location.  When a
> >    breakpoint is set on the "cold" address range, this is almost certainly
> >    incorrect.  The breakpoint should instead be set only on code near the
> >    entry point(s).
> > 
> > 3) The disassemble command did not work correctly.  E.g. here is what I
> >    found during my analysis of 84550:
> > 
> > 	(gdb) x/i 'main.cold.0'
> > 	   0x4010e0 <main()>:   mov    %rax,%rdi
> > 	(gdb) x/i main
> > 	   0x4011a0 <main>:     push   %r12
> > 	(gdb) disassemble main
> > 	Dump of assembler code for function main():
> > 	   0x00000000004010e0 <+0>:     mov    %rax,%rdi
> > 	   ...
> >         [No addresses starting at 0x4011a0 are shown]
> > 
> > 4) Display of addresses associated with the non-contiguous function are
> >    confusing.  E.g. in the above example, note that GDB thinks that
> >    the address associated with main.cold.0 is <main()>, but that there's
> >    also a minsym called main which is displayed as <main>.
> > 
> > There are probably several other problems which are related those
> > identified above.
> > 
> > I discovered that the stepping problem could be "fixed" by disabling
> > the find_pc_partial_function cache.  This cache keeps track of the
> > most recent result (of calling find_pc_partial_function).  If
> > find_pc_partial_function is called with an address which falls within
> > the cache range, then that's considered to be a cache hit and the most
> > recent result is returned.  Obviously, this won't work correctly for
> > functions which occupy non-contiguous (disjoint) address ranges where
> > other functions might be placed in the gap.
> > 
> > So one of the problems that needed to be solved was to make the
> > caching code work correctly.  It is interesting to note that stepping
> > _did_ work when the cache was disabled.  This is/was due to GDB
> > already having some (albeit incomplete) support for non-contiguous
> > addresses in the form of blockvector address maps.  Code responsible
> > for mapping addresses to blocks (which form the lower levels of
> > find_pc_partial_function) handle this case correctly.
> > 
> > To solve the problem of incorrect disassembly, we need to be able
> > to iterate over all of the ranges associated with a block.
> > 
> > Finally, we need to distinguish between the entry pc and the lowest
> > address in a block.  I discovered that this lack of distinction was
> > the cause of the remainder of the bugs including some which seemed to
> > be introduced by fixing the problems noted above.  Once this
> > distinction is made, it will be straightforward to add full support for
> > DW_AT_entry_pc.  I considered adding this support as part of this
> > patch series, but decided to wait until the community weighs in on my
> > work thus far...
> >   
> 
> Hi Kevin,
> 
> I just started looking into this, but I already have a question, just for
> the sake of the discussion.  After reading this description and the first
> few patches, it seems to me like one "cheap" way to fix the caching issue
> would be to record the contiguous portion of the address range instead
> of the low/high of the complete block.
> 
> So if we have this:
> 
>   [0x100,0x110) function_a
>   [0x110,0x120) function_b
>   [0x120,0x130) function_a.cold
> 
> When probing for pc = 0x104, we currently save:
> 
>   cache_pc_function_low = 0x100
>   cache_pc_function_high = 0x130
> 
> But instead we would save;
> 
>   cache_pc_function_low = 0x100
>   cache_pc_function_high = 0x110
> 
> I assume that the usage pattern of find_pc_partial_function is querying
> multiple times the same pc, or pc in increasing order (e.g. when
> disassembling).  This modification wouldn't change the efficiency of
> the cache in these situations.
> 
> Your solution of recording address ranges for blocks is more complete
> and probably necessary for other use cases, I am not questioning that,
> but I wanted to know if you had first tried a simpler solution similar
> to the above.
> 
> Simon

Hi Simon,

I haven't tried the simpler caching approach that you outline above. 
It seems like it should work and it would definitely make checking for
whether or not something is in the cache simpler.

I'll give it a try and run the test suite to see if there's a problem with
doing it that way.

Thanks,

Kevin

Re: [PATCH 0/8] Non-contiguous address range support

[Simon Marchi]

On 2018-07-16 22:00, Kevin Buettner wrote:
> Hi Simon,
> 
> I haven't tried the simpler caching approach that you outline above.
> It seems like it should work and it would definitely make checking for
> whether or not something is in the cache simpler.
> 
> I'll give it a try and run the test suite to see if there's a problem 
> with
> doing it that way.
> 
> Thanks,
> 
> Kevin

Just note that I don't have the complete picture yet.  If the block 
range information is necessary for some other reason, then it would make 
sense to use it as well for that cache.  I didn't want to send you on a 
long side-quest for nothing!

Simon

Re: [PATCH 3/8] Add support for non-contiguous blocks to find_pc_partial_function

[Kevin Buettner]

The description and patch below are intended as a replacement for my
original patch.  It uses the approach outlined by Simon Marchi for
checking the find_pc_partial_function cache.

- - - -

This change adds an optional output parameter BLOCK to
find_pc_partial_function.  If BLOCK is non-null, then *BLOCK will be
set to the address of the block corresponding to the function symbol
if such a symbol was found during lookup.  Otherwise it's set to a
NULL value.  Callers may wish to use the block information to
determine whether the block contains any non-contiguous ranges.  The
caller may also iterate over or examine those ranges.

When I first started looking at the broken stepping behavior associated
with functions w/ non-contiguous ranges, I found that I could "fix"
the problem by disabling the find_pc_partial_function cache.  It would
sometimes happen that the PC passed in would be between the low and
high cache values, but would be in some other function that happens to
be placed in between the ranges for the cached function.  This caused
incorrect values to be returned.

So dealing with this cache turns out to be very important for fixing
this problem.  I explored three different ways of dealing with the cache.

My first approach was to clear the cache when a block was encountered
with more than one range.  This would cause the non-cache pathway to
be executed on the next call to find_pc_partial_function.

Another approach, which I suspect is slightly faster, checks to see
whether the PC is within one of the ranges associated with the cached
block.  If so, then the cached values can be used.  It falls back to
the original behavior if there is no cached block.

The current approach, suggested by Simon Marchi, is to restrict the
low/high pc values recorded for the cache to the beginning and end of
the range containing the PC value under consideration.  This allows us
to retain the simple (and fast) test for determining whether the
memoized (cached) values apply to the PC passed to
find_pc_partial_function.

Another choice that had to be made was whether to have ADDRESS
continue to represent the lowest address associated with the function
or with the entry pc associated with the function.  For the moment,
I've decided to keep the current behavior of having it represent the
lowest address.  In cases where the entry pc is needed, this can be
found by passing a non-NULL value for BLOCK which will cause the block
(pointer) associated with the function to be returned.  BLOCK_ENTRY_PC
can then be used on that block to obtain the entry pc.

gdb/ChangeLog:
    
    	* symtab.h (find_pc_partial_function): Add new parameter `block'.
    	* blockframe.c (cache_pc_function_block): New static global.
    	(clear_pc_function_cache): Clear cache_pc_function_block.
    	(find_pc_partial_function): Move comment to symtab.h. Add support
    	for non-contiguous blocks.
---
 gdb/blockframe.c | 77 ++++++++++++++++++++++++++++++++++++++++----------------
 gdb/symtab.h     | 20 ++++++++++++---
 2 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index b3c9aa3..a3b2a11 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -159,6 +159,7 @@ static CORE_ADDR cache_pc_function_low = 0;
 static CORE_ADDR cache_pc_function_high = 0;
 static const char *cache_pc_function_name = 0;
 static struct obj_section *cache_pc_function_section = NULL;
+static const struct block *cache_pc_function_block = nullptr;
 
 /* Clear cache, e.g. when symbol table is discarded.  */
 
@@ -169,24 +170,14 @@ clear_pc_function_cache (void)
   cache_pc_function_high = 0;
   cache_pc_function_name = (char *) 0;
   cache_pc_function_section = NULL;
+  cache_pc_function_block = nullptr;
 }
 
-/* Finds the "function" (text symbol) that is smaller than PC but
-   greatest of all of the potential text symbols in SECTION.  Sets
-   *NAME and/or *ADDRESS conditionally if that pointer is non-null.
-   If ENDADDR is non-null, then set *ENDADDR to be the end of the
-   function (exclusive), but passing ENDADDR as non-null means that
-   the function might cause symbols to be read.  This function either
-   succeeds or fails (not halfway succeeds).  If it succeeds, it sets
-   *NAME, *ADDRESS, and *ENDADDR to real information and returns 1.
-   If it fails, it sets *NAME, *ADDRESS and *ENDADDR to zero and
-   returns 0.  */
-
-/* Backward compatibility, no section argument.  */
+/* See symtab.h.  */
 
 int
 find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
-			  CORE_ADDR *endaddr)
+			  CORE_ADDR *endaddr, const struct block **block)
 {
   struct obj_section *section;
   struct symbol *f;
@@ -232,13 +223,37 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
       f = find_pc_sect_function (mapped_pc, section);
       if (f != NULL
 	  && (msymbol.minsym == NULL
-	      || (BLOCK_START (SYMBOL_BLOCK_VALUE (f))
+	      || (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (f))
 		  >= BMSYMBOL_VALUE_ADDRESS (msymbol))))
 	{
-	  cache_pc_function_low = BLOCK_START (SYMBOL_BLOCK_VALUE (f));
-	  cache_pc_function_high = BLOCK_END (SYMBOL_BLOCK_VALUE (f));
+	  const struct block *b = SYMBOL_BLOCK_VALUE (f);
+
+	  if (BLOCK_CONTIGUOUS_P (b))
+	    {
+	      cache_pc_function_low = BLOCK_START (b);
+	      cache_pc_function_high = BLOCK_END (b);
+	    }
+	  else
+	    {
+	      int i;
+	      for (i = 0; i < BLOCK_NRANGES (b); i++)
+	        {
+		  if (BLOCK_RANGE_START (b, i) <= mapped_pc
+		      && mapped_pc < BLOCK_RANGE_END (b, i))
+		    {
+		      cache_pc_function_low = BLOCK_RANGE_START (b, i);
+		      cache_pc_function_high = BLOCK_RANGE_END (b, i);
+		      break;
+		    }
+		}
+	      /* Above loop should exit via the break.  */
+	      gdb_assert (i < BLOCK_NRANGES (b));
+	    }
+
 	  cache_pc_function_name = SYMBOL_LINKAGE_NAME (f);
 	  cache_pc_function_section = section;
+	  cache_pc_function_block = b;
+
 	  goto return_cached_value;
 	}
     }
@@ -268,15 +283,33 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
   cache_pc_function_name = MSYMBOL_LINKAGE_NAME (msymbol.minsym);
   cache_pc_function_section = section;
   cache_pc_function_high = minimal_symbol_upper_bound (msymbol);
+  cache_pc_function_block = nullptr;
 
  return_cached_value:
 
+  CORE_ADDR f_low, f_high;
+
+  /* The low and high addresses for the cache do not necessarily
+     correspond to the low and high addresses for the function.
+     Extract the function low/high addresses from the cached block
+     if there is one; otherwise use the cached low & high values.  */
+  if (cache_pc_function_block)
+    {
+      f_low = BLOCK_START (cache_pc_function_block);
+      f_high = BLOCK_END (cache_pc_function_block);
+    }
+  else
+    {
+      f_low = cache_pc_function_low;
+      f_high = cache_pc_function_high;
+    }
+
   if (address)
     {
       if (pc_in_unmapped_range (pc, section))
-	*address = overlay_unmapped_address (cache_pc_function_low, section);
+	*address = overlay_unmapped_address (f_low, section);
       else
-	*address = cache_pc_function_low;
+	*address = f_low;
     }
 
   if (name)
@@ -291,13 +324,15 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
 	     the overlay), we must actually convert (high - 1) and
 	     then add one to that.  */
 
-	  *endaddr = 1 + overlay_unmapped_address (cache_pc_function_high - 1,
-						   section);
+	  *endaddr = 1 + overlay_unmapped_address (f_high - 1, section);
 	}
       else
-	*endaddr = cache_pc_function_high;
+	*endaddr = f_high;
     }
 
+  if (block)
+    *block = cache_pc_function_block;
+
   return 1;
 }
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 84fc897..e4de868 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1683,10 +1683,22 @@ extern struct symbol *find_pc_sect_function (CORE_ADDR, struct obj_section *);
 
 extern struct symbol *find_symbol_at_address (CORE_ADDR);
 
-/* lookup function from address, return name, start addr and end addr.  */
-
-extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *,
-				     CORE_ADDR *);
+/* Finds the "function" (text symbol) that is smaller than PC but
+   greatest of all of the potential text symbols in SECTION.  Sets
+   *NAME and/or *ADDRESS conditionally if that pointer is non-null.
+   If ENDADDR is non-null, then set *ENDADDR to be the end of the
+   function (exclusive).  If the optional parameter BLOCK is non-null,
+   then set *BLOCK to the address of the block corresponding to the
+   function symbol, if such a symbol could be found during the lookup;
+   nullptr is used as a return value for *BLOCK if no block is found. 
+   This function either succeeds or fails (not halfway succeeds).  If
+   it succeeds, it sets *NAME, *ADDRESS, and *ENDADDR to real
+   information and returns 1.  If it fails, it sets *NAME, *ADDRESS
+   and *ENDADDR to zero and returns 0.  */
+
+extern int find_pc_partial_function (CORE_ADDR pc, const char **name,
+				     CORE_ADDR *address, CORE_ADDR *endaddr,
+				     const struct block **block = nullptr);
 
 /* Return the type of a function with its first instruction exactly at
    the PC address.  Return NULL otherwise.  */

Re: [PATCH 0/8] Non-contiguous address range support

[Kevin Buettner]

On Thu, 19 Jul 2018 11:55:48 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2018-07-16 22:00, Kevin Buettner wrote:
> > Hi Simon,
> > 
> > I haven't tried the simpler caching approach that you outline above.
> > It seems like it should work and it would definitely make checking for
> > whether or not something is in the cache simpler.
> > 
> > I'll give it a try and run the test suite to see if there's a problem 
> > with
> > doing it that way.
> > 
> > Thanks,
> > 
> > Kevin  
> 
> Just note that I don't have the complete picture yet.  If the block 
> range information is necessary for some other reason, then it would make 
> sense to use it as well for that cache.  I didn't want to send you on a 
> long side-quest for nothing!

Hi Simon,

The block range information is still required for several other reasons.
One obvious case is that we need to iterate through the ranges for
displaying disassembly.

However, I really liked your suggestion because we can retain the simple
test for determining whether or not PC is in the cache:

  if (mapped_pc >= cache_pc_function_low
      && mapped_pc < cache_pc_function_high
      && section == cache_pc_function_section)
    goto return_cached_value;

I've posted a replacement for patch 3/8 which uses your approach. 
Note that the block range information is still used within that patch
for determining the low and high addresses for the cache.

What's not in that patch is the removal of block_contains_pc which
was added in patch 1/8.  The only caller of this function was in
my earlier patch for find_pc_partial_function (in 3/8).  It's no
longer needed.

I'm guessing that you'll have some suggestions which will require
a v2 patch series.  I'll wait to show the removal of block_contains_pc
until that time.

Thanks for looking at this...

Kevin

Re: [PATCH 1/8] Add block range data structure for blocks with non-contiguous address ranges

[Simon Marchi]

On 2018-06-26 02:42 AM, Kevin Buettner wrote:
> This patch does the following:
> 
> - Introduces a block range data structure which is accessed via
>   a new field in struct block.
> - Defines several macros for accessing block ranges.
> - Defines a new function, make_blockrange, which is responsible for
>   creating the new data structure.
> - Defines a predicate (function) which checks to see if an address
>   is present in a block.
> 
> It should be noted that some support for non-contiguous ranges already
> existed in GDB in the form of blockvector addrmaps.  This support
> allowed GDB to quickly find a block containing a particular address
> even when the block consists of non-contiguous addresses.  See
> find_block_in_blockvector() in block.c, dwarf2_record_block_ranges()
> in dwarf2read.c, and record_block_range() in buildsym.c.
> 
> Addrmaps do not provide a convenient way to examine address ranges
> associated with a particular block.  This data structure (and its
> interface) is set up for quickly finding the value (which in this case
> is a block) associated with a particular address.  The interface
> does not include a method for doing a reverse mapping from blocks to
> addresses.  A linear time mapping might be attempted via use of the
> addrmap's foreach method, but this is not as straightforward as it
> might first appear due to the fact that blocks corresponding to inline
> function instances and lexical blocks w/ variables end up getting
> interspersed in in the set of transitions.
> 
> Note:  If this approach is deemed to be too expensive in terms of
> space, an alternate approach might be to attempt the linear time
> mapping noted above.  find_pc_partial_function() needs to be able to
> quickly know whether there are discontiguous ranges (for determining
> cache validity), so a flag for this property would have to be added to
> struct block.  Also integral to this set of changes is the concept of
> an "entry pc" which might be different than the block's start address.
> An entry_pc field would also need to be added to struct block.  This
> does not result in any space savings in struct block though since the
> space for the flag and entry_pc use more space than the blockranges
> struct pointer that I've added.  There would, however, be some space
> savings due to the fact that the new data structures that I've added
> for this patch would not need to be allocated.  (I happen to like the
> approach I've come up with, but I wanted to mention another possibility
> just in case someone does not.)

I assume the impact won't be too big (there probably isn't a ton of ranges
per block), but some actual data would be interesting.

Just two nits below, otherwise it LGTM.

> @@ -807,3 +822,27 @@ block_find_non_opaque_type_preferred (struct symbol *sym, void *data)
>    *best = sym;
>    return 0;
>  }
> +
> +/* See block.h.  */
> +
> +struct blockranges *
> +make_blockranges (struct objfile * objfile,

Spurious space after the *.

> @@ -322,4 +401,9 @@ extern int block_find_non_opaque_type_preferred (struct symbol *sym,
>         (sym) != NULL;							\
>         (sym) = block_iter_match_next ((name), &(iter)))
>  
> +/* Given a vector of pairs, allocate and build an obstack allocated
> +   blockranges struct for a block.  */
> +struct blockranges *make_blockranges (struct objfile *objfile,
> +                                      std::vector<std::pair<CORE_ADDR, CORE_ADDR>> &);

Can you please name the second parameter, and make it const?

Thanks,

Simon

Re: [PATCH 1/8] Add block range data structure for blocks with non-contiguous address ranges

[Simon Marchi]

On 2018-06-26 02:42 AM, Kevin Buettner wrote:
> @@ -322,4 +401,9 @@ extern int block_find_non_opaque_type_preferred (struct symbol *sym,
>         (sym) != NULL;							\
>         (sym) = block_iter_match_next ((name), &(iter)))
>  
> +/* Given a vector of pairs, allocate and build an obstack allocated
> +   blockranges struct for a block.  */
> +struct blockranges *make_blockranges (struct objfile *objfile,
> +                                      std::vector<std::pair<CORE_ADDR, CORE_ADDR>> &);

Oh, also, this vector could be a vector of struct blockrange.  It would be more
expressive than a vector of pairs.

Simon

Re: [PATCH 2/8] Record explicit block ranges from dwarf2read.c

[Simon Marchi]

On 2018-06-26 02:44 AM, Kevin Buettner wrote:
> This change sets BLOCK_RANGES for the block under consideration by
> calling make_blockranges().  This action is performed in
> dwarf2_record_block_ranges().
> 
> It should be noted that dwarf2_record_block_ranges() already does some
> recording of the range via a call to record_block_range().  The ranges
> recorded in that fashion end up in the address map associated with the
> blockvector for the compilation unit's symtab.  Given an address, the
> addrmap provides a fast way of finding the block containing that
> address.  The address map does not, however, provide a convenient way
> of determining which address ranges make up a particular block.
> 
> While reading a set of ranges, a vector of pairs is used to collect
> the starting and ending addresses for each range in the block.  Once
> all of the ranges for a block have been collected, make_blockranges()
> is called to fill in BLOCK_RANGES for the block.
> 
> The ranges are stored for the block in the order that they're read
> from the debug info.  For DWARF, the starting address of the first
> range of the block will be the entry pc in cases where DW_AT_entry_pc
> is not present.  (Well, that would ideally be the case.  At the moment
> DW_AT_entry_pc is not being handled.)
> 
> gdb/ChangeLog:
>     
>     	* dwarf2read.c (dwarf2_record_block_ranges): Fill in BLOCK_RANGES
>     	for block.
> ---
>  gdb/dwarf2read.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 4ad0527..2f0f9b9 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -14722,6 +14722,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>        unsigned long offset = (DW_UNSND (attr)
>  			      + (need_ranges_base ? cu->ranges_base : 0));
>  
> +      std::vector<std::pair<CORE_ADDR, CORE_ADDR>> blockvec;
>        dwarf2_ranges_process (offset, cu,
>  	[&] (CORE_ADDR start, CORE_ADDR end)
>  	{
> @@ -14730,7 +14731,10 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>  	  start = gdbarch_adjust_dwarf2_addr (gdbarch, start);
>  	  end = gdbarch_adjust_dwarf2_addr (gdbarch, end);
>  	  record_block_range (block, start, end - 1);
> +	  blockvec.push_back (std::make_pair (start, end));

A real small nit, using

  blockvec.emplace_back (start, end);

would avoid some unnecessary copying.  Otherwise, it LGTM.

Simon

Re: [PATCH 3/8] Add support for non-contiguous blocks to find_pc_partial_function

[Simon Marchi]

On 2018-07-19 02:52 PM, Kevin Buettner wrote:
> The description and patch below are intended as a replacement for my
> original patch.  It uses the approach outlined by Simon Marchi for
> checking the find_pc_partial_function cache.
> 
> - - - -
> 
> This change adds an optional output parameter BLOCK to
> find_pc_partial_function.  If BLOCK is non-null, then *BLOCK will be
> set to the address of the block corresponding to the function symbol
> if such a symbol was found during lookup.  Otherwise it's set to a
> NULL value.  Callers may wish to use the block information to
> determine whether the block contains any non-contiguous ranges.  The
> caller may also iterate over or examine those ranges.
> 
> When I first started looking at the broken stepping behavior associated
> with functions w/ non-contiguous ranges, I found that I could "fix"
> the problem by disabling the find_pc_partial_function cache.  It would
> sometimes happen that the PC passed in would be between the low and
> high cache values, but would be in some other function that happens to
> be placed in between the ranges for the cached function.  This caused
> incorrect values to be returned.
> 
> So dealing with this cache turns out to be very important for fixing
> this problem.  I explored three different ways of dealing with the cache.
> 
> My first approach was to clear the cache when a block was encountered
> with more than one range.  This would cause the non-cache pathway to
> be executed on the next call to find_pc_partial_function.
> 
> Another approach, which I suspect is slightly faster, checks to see
> whether the PC is within one of the ranges associated with the cached
> block.  If so, then the cached values can be used.  It falls back to
> the original behavior if there is no cached block.
> 
> The current approach, suggested by Simon Marchi, is to restrict the
> low/high pc values recorded for the cache to the beginning and end of
> the range containing the PC value under consideration.  This allows us
> to retain the simple (and fast) test for determining whether the
> memoized (cached) values apply to the PC passed to
> find_pc_partial_function.
> 
> Another choice that had to be made was whether to have ADDRESS
> continue to represent the lowest address associated with the function
> or with the entry pc associated with the function.  For the moment,
> I've decided to keep the current behavior of having it represent the
> lowest address.  In cases where the entry pc is needed, this can be
> found by passing a non-NULL value for BLOCK which will cause the block
> (pointer) associated with the function to be returned.  BLOCK_ENTRY_PC
> can then be used on that block to obtain the entry pc.

This LGTM overall, just a few nits/suggestions/questions.

> diff --git a/gdb/blockframe.c b/gdb/blockframe.c
> index b3c9aa3..a3b2a11 100644
> --- a/gdb/blockframe.c
> +++ b/gdb/blockframe.c
> @@ -159,6 +159,7 @@ static CORE_ADDR cache_pc_function_low = 0;
>  static CORE_ADDR cache_pc_function_high = 0;
>  static const char *cache_pc_function_name = 0;
>  static struct obj_section *cache_pc_function_section = NULL;
> +static const struct block *cache_pc_function_block = nullptr;
>  
>  /* Clear cache, e.g. when symbol table is discarded.  */
>  
> @@ -169,24 +170,14 @@ clear_pc_function_cache (void)
>    cache_pc_function_high = 0;
>    cache_pc_function_name = (char *) 0;
>    cache_pc_function_section = NULL;
> +  cache_pc_function_block = nullptr;

We might want to rename cache_pc_function_low and cache_pc_function_high to
reflect their new usage (since they don't represent the low/high bounds of
the function anymore, but the range).

Alternatively, it might make things simpler to keep cache_pc_function_low
and cache_pc_function_high as they are right now, and introduce two
new variables for the bounds of the matched range.

>  }
>  
> -/* Finds the "function" (text symbol) that is smaller than PC but
> -   greatest of all of the potential text symbols in SECTION.  Sets
> -   *NAME and/or *ADDRESS conditionally if that pointer is non-null.
> -   If ENDADDR is non-null, then set *ENDADDR to be the end of the
> -   function (exclusive), but passing ENDADDR as non-null means that
> -   the function might cause symbols to be read.  This function either
> -   succeeds or fails (not halfway succeeds).  If it succeeds, it sets
> -   *NAME, *ADDRESS, and *ENDADDR to real information and returns 1.
> -   If it fails, it sets *NAME, *ADDRESS and *ENDADDR to zero and
> -   returns 0.  */
> -
> -/* Backward compatibility, no section argument.  */
> +/* See symtab.h.  */
>  
>  int
>  find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> -			  CORE_ADDR *endaddr)
> +			  CORE_ADDR *endaddr, const struct block **block)
>  {
>    struct obj_section *section;
>    struct symbol *f;
> @@ -232,13 +223,37 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>        f = find_pc_sect_function (mapped_pc, section);
>        if (f != NULL
>  	  && (msymbol.minsym == NULL
> -	      || (BLOCK_START (SYMBOL_BLOCK_VALUE (f))
> +	      || (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (f))

I don't understand this change, can you explain it briefly?

>  		  >= BMSYMBOL_VALUE_ADDRESS (msymbol))))
>  	{
> -	  cache_pc_function_low = BLOCK_START (SYMBOL_BLOCK_VALUE (f));
> -	  cache_pc_function_high = BLOCK_END (SYMBOL_BLOCK_VALUE (f));
> +	  const struct block *b = SYMBOL_BLOCK_VALUE (f);
> +
> +	  if (BLOCK_CONTIGUOUS_P (b))
> +	    {
> +	      cache_pc_function_low = BLOCK_START (b);
> +	      cache_pc_function_high = BLOCK_END (b);
> +	    }
> +	  else
> +	    {
> +	      int i;
> +	      for (i = 0; i < BLOCK_NRANGES (b); i++)
> +	        {
> +		  if (BLOCK_RANGE_START (b, i) <= mapped_pc
> +		      && mapped_pc < BLOCK_RANGE_END (b, i))
> +		    {
> +		      cache_pc_function_low = BLOCK_RANGE_START (b, i);
> +		      cache_pc_function_high = BLOCK_RANGE_END (b, i);
> +		      break;
> +		    }
> +		}
> +	      /* Above loop should exit via the break.  */
> +	      gdb_assert (i < BLOCK_NRANGES (b));
> +	    }
> +
>  	  cache_pc_function_name = SYMBOL_LINKAGE_NAME (f);
>  	  cache_pc_function_section = section;
> +	  cache_pc_function_block = b;
> +
>  	  goto return_cached_value;
>  	}
>      }
> @@ -268,15 +283,33 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>    cache_pc_function_name = MSYMBOL_LINKAGE_NAME (msymbol.minsym);
>    cache_pc_function_section = section;
>    cache_pc_function_high = minimal_symbol_upper_bound (msymbol);
> +  cache_pc_function_block = nullptr;
>  
>   return_cached_value:
>  
> +  CORE_ADDR f_low, f_high;
> +
> +  /* The low and high addresses for the cache do not necessarily
> +     correspond to the low and high addresses for the function.
> +     Extract the function low/high addresses from the cached block
> +     if there is one; otherwise use the cached low & high values.  */
> +  if (cache_pc_function_block)

if (cache_pc_function_block != nullptr)

> +    {
> +      f_low = BLOCK_START (cache_pc_function_block);
> +      f_high = BLOCK_END (cache_pc_function_block);
> +    }
> +  else
> +    {
> +      f_low = cache_pc_function_low;
> +      f_high = cache_pc_function_high;
> +    }

This, for example, could probably be kept simple if new variables were
introduced for the matched block range, and cache_pc_function_{low,high}
stayed as-is.  I haven't actually tried, so it may not be a good idea at
all.

> +
>    if (address)
>      {
>        if (pc_in_unmapped_range (pc, section))
> -	*address = overlay_unmapped_address (cache_pc_function_low, section);
> +	*address = overlay_unmapped_address (f_low, section);
>        else
> -	*address = cache_pc_function_low;
> +	*address = f_low;
>      }
>  
>    if (name)
> @@ -291,13 +324,15 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
>  	     the overlay), we must actually convert (high - 1) and
>  	     then add one to that.  */
>  
> -	  *endaddr = 1 + overlay_unmapped_address (cache_pc_function_high - 1,
> -						   section);
> +	  *endaddr = 1 + overlay_unmapped_address (f_high - 1, section);
>  	}
>        else
> -	*endaddr = cache_pc_function_high;
> +	*endaddr = f_high;
>      }
>  
> +  if (block)

if (block != nullptr)

Thanks,

Simon

Re: [PATCH 4/8] Disassemble blocks with non-contiguous ranges

[Simon Marchi]

On 2018-06-26 02:49 AM, Kevin Buettner wrote:
> This patch adds support for disassembly of blocks with non-contiguous
> ranges.  These blocks are printed as follows:
> 
> (gdb) disassemble foo
> Dump of assembler code for function foo:
> Address range 0x401136 to 0x401151:
>    0x0000000000401136 <+0>:     push   %rbp
>    0x0000000000401137 <+1>:     mov    %rsp,%rbp
>    0x000000000040113a <+4>:     callq  0x401134 <bar>
>    0x000000000040113f <+9>:     mov    0x2eef(%rip),%eax        # 0x404034 <e>
>    0x0000000000401145 <+15>:    test   %eax,%eax
>    0x0000000000401147 <+17>:    je     0x40114e <foo+24>
>    0x0000000000401149 <+19>:    callq  0x401128 <foo+4294967282>
>    0x000000000040114e <+24>:    nop
>    0x000000000040114f <+25>:    pop    %rbp
>    0x0000000000401150 <+26>:    retq
> Address range 0x401128 to 0x401134:
>    0x0000000000401128 <+-14>:   push   %rbp
>    0x0000000000401129 <+-13>:   mov    %rsp,%rbp
>    0x000000000040112c <+-10>:   callq  0x401126 <baz>
>    0x0000000000401131 <+-5>:    nop
>    0x0000000000401132 <+-4>:    pop    %rbp
>    0x0000000000401133 <+-3>:    retq
> End of assembler dump.
> 
> This is an actual dump from the test case that I constructed for
> this work.  The ranges are printed in the order encountered in the
> debug info. For the above example, note that the second range occupies
> lower addresses than the first range.
> 
> Functions with contiguous ranges are still printed as follows:
> 
> (gdb) disassemble main
> Dump of assembler code for function main:
>    0x0000000000401151 <+0>:     push   %rbp
>    0x0000000000401152 <+1>:     mov    %rsp,%rbp
>    0x0000000000401155 <+4>:     callq  0x401136 <foo>
>    0x000000000040115a <+9>:     mov    $0x0,%eax
>    0x000000000040115f <+14>:    pop    %rbp
>    0x0000000000401160 <+15>:    retq
> End of assembler dump.
> 
> gdb/ChangeLog:
>     
>     	* cli/cli-cmds.c (block.h): Include.
>     	(print_disassembly): Handle printing of non-contiguous blocks.
>     	(disassemble_current_function): Likewise.
>     	(disassemble_command): Likewise.
> ---
>  gdb/cli/cli-cmds.c | 40 +++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 5c5d6dc..171936c 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -38,6 +38,7 @@
>  #include "tracepoint.h"
>  #include "filestuff.h"
>  #include "location.h"
> +#include "block.h"
>  
>  #include "ui-out.h"
>  
> @@ -1096,6 +1097,7 @@ list_command (const char *arg, int from_tty)
>  static void
>  print_disassembly (struct gdbarch *gdbarch, const char *name,
>  		   CORE_ADDR low, CORE_ADDR high,
> +		   const struct block *b,
>  		   gdb_disassembly_flags flags)

Could you document the new parameter?

>  {
>  #if defined(TUI)
> @@ -1104,14 +1106,28 @@ print_disassembly (struct gdbarch *gdbarch, const char *name,
>      {
>        printf_filtered ("Dump of assembler code ");
>        if (name != NULL)
> -        printf_filtered ("for function %s:\n", name);
> -      else
> -        printf_filtered ("from %s to %s:\n",
> -			 paddress (gdbarch, low), paddress (gdbarch, high));
> -
> -      /* Dump the specified range.  */
> -      gdb_disassembly (gdbarch, current_uiout, flags, -1, low, high);
> +	printf_filtered ("for function %s:\n", name);
> +      if (!b || BLOCK_CONTIGUOUS_P (b))

b == nullptr

> +        {
> +	  if (name == NULL)
> +	    printf_filtered ("from %s to %s:\n",
> +			     paddress (gdbarch, low), paddress (gdbarch, high));
>  
> +	  /* Dump the specified range.  */
> +	  gdb_disassembly (gdbarch, current_uiout, flags, -1, low, high);
> +	}
> +      else
> +        {
> +	  for (int i = 0; i < BLOCK_NRANGES (b); i++)
> +	    {
> +	      CORE_ADDR low = BLOCK_RANGE_START (b, i);
> +	      CORE_ADDR high = BLOCK_RANGE_END (b, i);
> +	      printf_filtered ("Address range %s to %s:\n",
> +			       paddress (gdbarch, low),
> +			       paddress (gdbarch, high));

Use _() for user-visible strings.  You can add it to pre-existing printfs that
your patch touches, if you want.

Otherwise, LGTM.

Simon

Re: [PATCH 5/8] Use BLOCK_ENTRY_PC in place of most uses of BLOCK_START

[Simon Marchi]

On 2018-06-26 02:51 AM, Kevin Buettner wrote:
> This change/patch substitues BLOCK_ENTRY_PC for BLOCK_START in
> places where BLOCK_START is used to obtain the address at which
> execution should enter the block.  Since blocks can now contain
> non-contiguous ranges, the BLOCK_START - which is still be the
> very lowest address in the block - might not be the same as
> BLOCK_ENTRY_PC.
> 
> There is a change to infrun.c which is less obvious and less mechanical.
> I'm posting it as a separate patch.

Hi Kevin,

I haven't "gotten" yet when we want to use BLOCK_START and when we want
to use BLOCK_ENTRY_PC.  I understand the difference between them, but
don't quite understand how to know which is the one we want.  It might
become clearer as I keep reading.  I trust you know what you are doing
anyway, so I assume the patch is good :).

Simon

Re: [PATCH 6/8] Use BLOCK_ENTRY_PC to find function entry pc in infrun.c

[Simon Marchi]

On 2018-06-26 02:53 AM, Kevin Buettner wrote:
> find_pc_partial_function() still returns the lowest and highest address
> of a function even when that function contains non-contiguous ranges.
> In cases where greater discrimination is required, the BLOCK parameter
> may be examined to determine the actual entry PC.
> 
> This patch uses the BLOCK return value from find_pc_partial_function()
> to obtain the entry PC. If no block is found - which can happen when
> only minimal symbols are available - then the start address provided
> by find_pc_partial_function() will still be used (as before).

This one looks good to me, given how the stop_func_start field seems to
be used.  From what I understand, it is used as the entry point of the
function, therefore BLOCK_ENTRY_PC seems appropriate.

> @@ -4297,10 +4297,20 @@ fill_in_stop_func (struct gdbarch *gdbarch,
>  {
>    if (!ecs->stop_func_filled_in)
>      {
> +      const struct block *block;
> +
>        /* Don't care about return value; stop_func_start and stop_func_name
>  	 will both be 0 if it doesn't work.  */
>        find_pc_partial_function (stop_pc, &ecs->stop_func_name,
> -				&ecs->stop_func_start, &ecs->stop_func_end);
> +				&ecs->stop_func_start, &ecs->stop_func_end,
> +				&block);
> +
> +      /* If a block is returned, prefer the block's entry point instead of
> +         the lowest address of the block - these aren't necessarily the
> +	 same.  */
> +      if (block)

block != nullptr

Simon

Re: [PATCH 7/8] Relocate block range start and end addresses

[Simon Marchi]

On 2018-06-26 02:55 AM, Kevin Buettner wrote:
> gdb/ChangeLog:
>     
>     	* objfiles.c (objfile_relocate1): Relocate start and end addresses
>     	for each range in a block.
> ---
>  gdb/objfiles.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 95c39cf..21ca111 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -831,6 +831,14 @@ objfile_relocate1 (struct objfile *objfile,
>  	  BLOCK_START (b) += ANOFFSET (delta, block_line_section);
>  	  BLOCK_END (b) += ANOFFSET (delta, block_line_section);
>  
> +	  if (BLOCK_RANGES (b))

!= nullptr

Otherwise, LGTM.

Simon

Re: [PATCH 8/8] Test case for functions with non-contiguous ranges

[Simon Marchi]

On 2018-06-26 02:57 AM, Kevin Buettner wrote:
> See comments in the new files for what this is about - I tried to
> explain it all there.

This test looks really nice and thorough.

> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    verbose "Skipping DW_AT_ranges test."

I think "unsupported" would be more appropriate.

> +    return 0
> +}
> +
> +# The .c files use __attribute__.

This comment seems wrong/outdated.

> +if [get_compiler_info] {
> +    return -1
> +}
> +if !$gcc_compiled {
> +    verbose "Skipping DW_AT_ranges test."

"unsupported" here too?

> +    return 0
> +}
>
> +
> +standard_testfile dw2-ranges-func.c dw2-ranges-func-dw.S
> +
> +# We need to know the size of integer and address types in order to
> +# write some of the debugging info we'd like to generate.
> +#
> +# For that, we ask GDB by debugging our test program.  Any program
> +# would do, but since we already have it specifically for this
> +# testcase, might as well use that.
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile srcfile2
> +    declare_labels integer_label volatile_label ranges_label ranges2_label L
> +    set int_size [get_sizeof "int" 4]
> +
> +    # Find start address and length for our functions.
> +    set main_func \
> +	[function_range main [list ${srcdir}/${subdir}/$srcfile]]
> +    set foo_func \
> +	[function_range foo [list ${srcdir}/${subdir}/$srcfile]]
> +    set foo_low_func \
> +	[function_range foo_low [list ${srcdir}/${subdir}/$srcfile]]
> +    set bar_func \
> +	[function_range bar [list ${srcdir}/${subdir}/$srcfile]]
> +    set baz_func \
> +	[function_range baz [list ${srcdir}/${subdir}/$srcfile]]
> +
> +    set main_start [lindex $main_func 0]
> +    set main_len [lindex $main_func 1]
> +    set foo_start [lindex $foo_func 0]
> +    set foo_end {$foo_start + [lindex $foo_func 1]}
> +    set foo_low_start [lindex $foo_low_func 0]
> +    set foo_low_len [lindex $foo_low_func 1]
> +    set foo_low_end {$foo_low_start + $foo_low_len}
> +    set bar_start [lindex $bar_func 0]
> +    set bar_len [lindex $bar_func 1]
> +    set baz_start [lindex $baz_func 0]
> +    set baz_len [lindex $baz_func 1]

If you want to make this shorter, you could use lassign:

  https://www.tcl.tk/man/tcl/TclCmd/lassign.htm

Something like:

  lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
    main_start main_len

I see that you use expressions like " [lindex $main_func 0]" below, it would
be clearer to always use the *_start or *_len variables.

> +    # Generate ranges data.
> +    ranges {is_64 [is_64_target]} {
> +	ranges_label: sequence {
> +	    {range {$foo_start } $foo_end}
> +	    {range {$foo_low_start} $foo_low_end}
> +	}
> +	ranges2_label: sequence {
> +	    {range {$foo_start } $foo_end}
> +	    {range {$foo_low_start} $foo_low_end}
> +	    {range {$main_start} $main_start + $main_len}
> +	    {range {$bar_start} $bar_start + $bar_len}
> +	    {range {$baz_start} $baz_start + $baz_len}
> +	}

ranges_label and ranges2_label could perhaps have more expressive names?

> +set test "foo and foo_low are at different addresses"
> +if {$foo_low_addr == $foo_addr} {
> +    fail $test
> +} else {
> +    pass $test
> +}

This can be

  gdb_assert {$foo_low_addr != $foo_addr} "foo and foo_low are at different addresses"

> +
> +# This more permissive RE for "break foo" will allow a breakpoint on
> +# multiple locations to PASS.  */
> +gdb_test "break foo" \
> +    "Breakpoint.*at.*" \
> +    "break foo (2nd time)"

We should avoid having trailing parenthesis in test messages:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

In a test like yours, that is really multiple independent scenarios tested one
after the other, I like to structure the test like this:

proc_with_prefix test_something {} {
  ...
}

proc_with_prefix test_something_else {} {
  ...
}

test_something
test_something_else


Using proc_with_prefix automatically prefixes test names with the name of the
proc (which is usually self-explanatory).  This avoids have to manually
differentiate test names such as in "break foo (2nd time)".

I also like to do a clean_restart at the beginning of each test procedure, to
reduce the chance of inter-dependency between each test procedure, though it's
not strictly necessary.

Simon

Re: [PATCH 3/8] Add support for non-contiguous blocks to find_pc_partial_function

[Kevin Buettner]

On Tue, 31 Jul 2018 21:59:57 -0400
Simon Marchi <simon.marchi@ericsson.com> wrote:

> >  int
> >  find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> > -			  CORE_ADDR *endaddr)
> > +			  CORE_ADDR *endaddr, const struct block **block)
> >  {
> >    struct obj_section *section;
> >    struct symbol *f;
> > @@ -232,13 +223,37 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> >        f = find_pc_sect_function (mapped_pc, section);
> >        if (f != NULL
> >  	  && (msymbol.minsym == NULL
> > -	      || (BLOCK_START (SYMBOL_BLOCK_VALUE (f))
> > +	      || (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (f))  
> 
> I don't understand this change, can you explain it briefly?
> 
> >  		  >= BMSYMBOL_VALUE_ADDRESS (msymbol))))  
> >  	{

A function's minimal symbol generally (maybe even always) refers to
the entry pc.  Therefore, we want to compare the block's entry pc to
the minimal symbol address instead of the block start - which might
not be the same as the entry pc.  BLOCK_START will refer to the lowest
address in all of the ranges.

I'll add a comment to the code explaining this.

Kevin

Re: [PATCH 1/8] Add block range data structure for blocks with non-contiguous address ranges

[Kevin Buettner]

On Tue, 31 Jul 2018 21:35:50 -0400
Simon Marchi <simon.marchi@ericsson.com> wrote:

> On 2018-06-26 02:42 AM, Kevin Buettner wrote:
> > This patch does the following:
> > 
> > - Introduces a block range data structure which is accessed via
> >   a new field in struct block.
> > - Defines several macros for accessing block ranges.
> > - Defines a new function, make_blockrange, which is responsible for
> >   creating the new data structure.
> > - Defines a predicate (function) which checks to see if an address
> >   is present in a block.
> > 
> > It should be noted that some support for non-contiguous ranges already
> > existed in GDB in the form of blockvector addrmaps.  This support
> > allowed GDB to quickly find a block containing a particular address
> > even when the block consists of non-contiguous addresses.  See
> > find_block_in_blockvector() in block.c, dwarf2_record_block_ranges()
> > in dwarf2read.c, and record_block_range() in buildsym.c.
> > 
> > Addrmaps do not provide a convenient way to examine address ranges
> > associated with a particular block.  This data structure (and its
> > interface) is set up for quickly finding the value (which in this case
> > is a block) associated with a particular address.  The interface
> > does not include a method for doing a reverse mapping from blocks to
> > addresses.  A linear time mapping might be attempted via use of the
> > addrmap's foreach method, but this is not as straightforward as it
> > might first appear due to the fact that blocks corresponding to inline
> > function instances and lexical blocks w/ variables end up getting
> > interspersed in in the set of transitions.
> > 
> > Note:  If this approach is deemed to be too expensive in terms of
> > space, an alternate approach might be to attempt the linear time
> > mapping noted above.  find_pc_partial_function() needs to be able to
> > quickly know whether there are discontiguous ranges (for determining
> > cache validity), so a flag for this property would have to be added to
> > struct block.  Also integral to this set of changes is the concept of
> > an "entry pc" which might be different than the block's start address.
> > An entry_pc field would also need to be added to struct block.  This
> > does not result in any space savings in struct block though since the
> > space for the flag and entry_pc use more space than the blockranges
> > struct pointer that I've added.  There would, however, be some space
> > savings due to the fact that the new data structures that I've added
> > for this patch would not need to be allocated.  (I happen to like the
> > approach I've come up with, but I wanted to mention another possibility
> > just in case someone does not.)  
> 
> I assume the impact won't be too big (there probably isn't a ton of ranges
> per block), but some actual data would be interesting.

In real code, I've seen at most two ranges per block.  At the moment,
I don't think that GCC is overly aggressive at splitting functions into
multiple ranges.  Most functions occupy just one range.

Jakub's test case called abort().  The function in which abort() appeared
was split into two ranges.  One range contained the call to abort; the other
had the rest of the function.

I've also seen C++ functions with try/catch broken up into more than one
range.  The less frequently used case is placed within a "cold" range.

Kevin

Re: [PATCH 5/8] Use BLOCK_ENTRY_PC in place of most uses of BLOCK_START

[Kevin Buettner]

On Tue, 31 Jul 2018 22:21:46 -0400
Simon Marchi <simon.marchi@ericsson.com> wrote:

> On 2018-06-26 02:51 AM, Kevin Buettner wrote:
> > This change/patch substitues BLOCK_ENTRY_PC for BLOCK_START in
> > places where BLOCK_START is used to obtain the address at which
> > execution should enter the block.  Since blocks can now contain
> > non-contiguous ranges, the BLOCK_START - which is still be the
> > very lowest address in the block - might not be the same as
> > BLOCK_ENTRY_PC.
> > 
> > There is a change to infrun.c which is less obvious and less mechanical.
> > I'm posting it as a separate patch.  
> 
> Hi Kevin,
> 
> I haven't "gotten" yet when we want to use BLOCK_START and when we want
> to use BLOCK_ENTRY_PC.  I understand the difference between them, but
> don't quite understand how to know which is the one we want.  It might
> become clearer as I keep reading.  I trust you know what you are doing
> anyway, so I assume the patch is good :).

Once these patches go in, use of BLOCK_START will almost always be
wrong.

Exceptions are instances where you're doing some manipulations of the
block or cases where you (for some reason) need to know the lowest
address in the block.

Kevin