Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [unavailable values part 1, 03/17] expose list of available ranges to common code
@ 2011-02-07 14:29 Pedro Alves
  2011-02-14 11:59 ` Jan Kratochvil
  0 siblings, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2011-02-07 14:29 UTC (permalink / raw)
  To: gdb-patches

Making use of the new traceframe info object, this patch add a
new function that exports a list of available memory ranges that
has been collected in the traceframe to common code.  No uses
are added in this patch yet though.

[I could have merged this with other patches,
but it was useful while developing to keep it separate...]

This splits the mem_range type into its own file,
as otherwise, a following patch would either need to make
exec.h include tracepoint.h (weird), or I'd have to move
the struct mem_range declaration somewhere more central,
like target.h (weird) or defs.h (desirably avoidable).

For the record, I tried making all this range handling be
done with addrmaps, but, I found out it was much more
trouble than it's worth.

-- 
Pedro Alves

2011-02-07  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* Makefile.in (SFILES): Add memrange.c.
	(HFILES_NO_SRCDIR): Add memrange.h.
	(COMMON_OBS): Add memrange.o.
	* memrange.c: New file.
	* memrange.h: New file.
	* tracepoint.c: Include memrange.h.
	(struct mem_range): Delete.
	(mem_range_s): Delete.
	(traceframe_available_memory): New function.
	* tracepoint.h (traceframe_available_memory): Declare.

---
 gdb/Makefile.in  |    6 ++--
 gdb/memrange.c   |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/memrange.h   |   51 ++++++++++++++++++++++++++++++++++
 gdb/tracepoint.c |   59 +++++++++++++++++++++++++++++----------
 gdb/tracepoint.h |    4 ++
 5 files changed, 184 insertions(+), 18 deletions(-)

Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2011-02-07 13:16:08.446706002 +0000
+++ src/gdb/Makefile.in	2011-02-07 13:16:55.856705999 +0000
@@ -711,7 +711,7 @@ SFILES = ada-exp.y ada-lang.c ada-typepr
 	m2-exp.y m2-lang.c m2-typeprint.c m2-valprint.c \
 	macrotab.c macroexp.c macrocmd.c macroscope.c main.c maint.c \
 	mdebugread.c memattr.c mem-break.c minsyms.c mipsread.c memory-map.c \
-	mi/mi-common.c \
+	memrange.c mi/mi-common.c \
 	objc-exp.y objc-lang.c \
 	objfiles.c osabi.c observer.c osdata.c \
 	opencl-lang.c \
@@ -770,7 +770,7 @@ gdbserver/mem-break.h gdbserver/wincecom
 gdbserver/linux-low.h gdbserver/gdb_proc_service.h \
 gdbserver/regcache.h gdbthread.h dwarf2-frame.h nbsd-nat.h dcache.h \
 amd64-nat.h s390-tdep.h arm-linux-tdep.h exceptions.h macroscope.h \
-gdbarch.h bsd-uthread.h gdb_thread_db.h gdb_stat.h memory-map.h	\
+gdbarch.h bsd-uthread.h gdb_thread_db.h gdb_stat.h memory-map.h	memrange.h \
 mdebugread.h m88k-tdep.h stabsread.h hppa-linux-offsets.h linux-fork.h \
 ser-unix.h inf-ptrace.h terminal.h ui-out.h frame-base.h \
 f-lang.h dwarf2loc.h value.h sparc-tdep.h defs.h target-descriptions.h \
@@ -889,7 +889,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $
 	trad-frame.o \
 	tramp-frame.o \
 	solib.o solib-target.o \
-	prologue-value.o memory-map.o xml-support.o xml-syscall.o \
+	prologue-value.o memory-map.o memrange.o xml-support.o xml-syscall.o \
 	target-descriptions.o target-memory.o xml-tdesc.o xml-builtin.o \
 	inferior.o osdata.o gdb_usleep.o record.o gcore.o \
 	jit.o progspace.o
Index: src/gdb/memrange.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/memrange.c	2011-02-07 13:17:12.586706003 +0000
@@ -0,0 +1,82 @@
+/* Memory ranges
+
+   Copyright (C) 2010, 2011 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "defs.h"
+#include "memrange.h"
+
+int
+mem_ranges_overlap (CORE_ADDR start1, int len1,
+		    CORE_ADDR start2, int len2)
+{
+  ULONGEST h, l;
+
+  l = max (start1, start2);
+  h = min (start1 + len1, start2 + len2);
+  return (l < h);
+}
+
+/* qsort comparison function, that compares mem_ranges.  */
+
+static int
+compare_mem_ranges (const void *ap, const void *bp)
+{
+  const struct mem_range *r1 = ap;
+  const struct mem_range *r2 = bp;
+
+  if (r1->start > r2->start)
+    return 1;
+  else if (r1->start < r2->start)
+    return -1;
+  else
+    return 0;
+}
+
+void
+normalize_mem_ranges (VEC(mem_range_s) *ranges)
+{
+  if (!VEC_empty (mem_range_s, ranges))
+    {
+      struct mem_range *ra, *rb;
+      int a, b;
+
+      qsort (VEC_address (mem_range_s, ranges),
+	     VEC_length (mem_range_s, ranges),
+	     sizeof (mem_range_s),
+	     compare_mem_ranges);
+
+      a = 0;
+      ra = VEC_index (mem_range_s, ranges, a);
+      for (b = 1; VEC_iterate (mem_range_s, ranges, b, rb); b++)
+	{
+	  /* If mem_range B overlaps or is adjacent to mem_range A,
+	     merge them.  */
+	  if (rb->start <= ra->start + ra->length)
+	    {
+	      ra->length = (rb->start + rb->length) - ra->start;
+	      continue;		/* next b, same a */
+	    }
+	  a++;			/* next a */
+	  ra = VEC_index (mem_range_s, ranges, a);
+
+	  if (a != b)
+	    *ra = *rb;
+	}
+      VEC_truncate (mem_range_s, ranges, a + 1);
+    }
+}
Index: src/gdb/memrange.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/memrange.h	2011-02-07 13:17:20.796706003 +0000
@@ -0,0 +1,51 @@
+/* The memory range data structure, and associated utilities.
+
+   Copyright (C) 2010, 2011 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef MEMRANGE_H
+#define MEMRANGE_H
+
+#include "vec.h"
+
+/* Defines a [START, START + LENGTH) memory range.  */
+
+struct mem_range
+{
+  /* Lowest address in the range.  */
+  CORE_ADDR start;
+
+  /* Length of the range.  */
+  int length;
+};
+
+typedef struct mem_range mem_range_s;
+
+DEF_VEC_O(mem_range_s);
+
+/* Returns true if the ranges defined by [start1, start1+len1) and
+   [start2, start2+len2) overlap.  */
+
+extern int mem_ranges_overlap (CORE_ADDR start1, int len1,
+			       CORE_ADDR start2, int len2);
+
+/* Sort ranges by start address, then coalesce contiguous or
+   overlapping ranges.  */
+
+extern void normalize_mem_ranges (VEC(mem_range_s) *memory);
+
+#endif
Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2011-02-07 13:16:08.446706002 +0000
+++ src/gdb/tracepoint.c	2011-02-07 13:16:55.866706000 +0000
@@ -50,6 +50,7 @@
 #include "source.h"
 #include "ax.h"
 #include "ax-gdb.h"
+#include "memrange.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -130,21 +131,6 @@ extern void output_command (char *, int)
 typedef struct trace_state_variable tsv_s;
 DEF_VEC_O(tsv_s);
 
-/* Defines a [START, START + LENGTH) memory range.  */
-
-struct mem_range
-{
-  /* Lowest address in the range.  */
-  CORE_ADDR start;
-
-  /* Length of the range.  */
-  int length;
-};
-
-typedef struct mem_range mem_range_s;
-
-DEF_VEC_O(mem_range_s);
-
 /* An object describing the contents of a traceframe.  */
 
 struct traceframe_info
@@ -4597,6 +4583,49 @@ get_traceframe_info (void)
   return traceframe_info;
 }
 
+/* Return in RESULT, the set of collected memory in the current
+   traceframe, found within the LEN bytes range starting at MEMADDR.
+   Returns true if the target supports the query, otherwise returns
+   false.  */
+
+int
+traceframe_available_memory (VEC(mem_range_s) **result,
+			     CORE_ADDR memaddr, ULONGEST len)
+{
+  struct traceframe_info *info = get_traceframe_info ();
+
+  if (info != NULL)
+    {
+      struct mem_range *r;
+      int i;
+
+      *result = NULL;
+
+      for (i = 0; VEC_iterate (mem_range_s, info->memory, i, r); i++)
+	if (mem_ranges_overlap (r->start, r->length, memaddr, len))
+	  {
+	    ULONGEST lo1, hi1, lo2, hi2;
+	    struct mem_range *nr;
+
+	    lo1 = memaddr;
+	    hi1 = memaddr + len;
+
+	    lo2 = r->start;
+	    hi2 = r->start + r->length;
+
+	    nr = VEC_safe_push (mem_range_s, *result, NULL);
+
+	    nr->start = max (lo1, lo2);
+	    nr->length = min (hi1, hi2) - nr->start;
+	  }
+
+      normalize_mem_ranges (*result);
+      return 1;
+    }
+
+  return 0;
+}
+
 /* module initialization */
 void
 _initialize_tracepoint (void)
Index: src/gdb/tracepoint.h
===================================================================
--- src.orig/gdb/tracepoint.h	2011-02-07 13:16:08.436706001 +0000
+++ src/gdb/tracepoint.h	2011-02-07 13:16:55.866706000 +0000
@@ -22,6 +22,7 @@
 
 #include "breakpoint.h"
 #include "target.h"
+#include "memrange.h"
 
 /* A trace state variable is a value managed by a target being
    traced.  A trace state variable (or tsv for short) can be accessed
@@ -238,4 +239,7 @@ extern void trace_save (const char *file
 
 extern struct traceframe_info *parse_traceframe_info (const char *tframe_info);
 
+extern int traceframe_available_memory (VEC(mem_range_s) **result,
+					CORE_ADDR memaddr, ULONGEST len);
+
 #endif	/* TRACEPOINT_H */


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

* Re: [unavailable values part 1, 03/17] expose list of available ranges to common code
  2011-02-07 14:29 [unavailable values part 1, 03/17] expose list of available ranges to common code Pedro Alves
@ 2011-02-14 11:59 ` Jan Kratochvil
  2011-02-14 19:54   ` Pedro Alves
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Kratochvil @ 2011-02-14 11:59 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Mon, 07 Feb 2011 15:29:18 +0100, Pedro Alves wrote:
> For the record, I tried making all this range handling be
> done with addrmaps, but, I found out it was much more
> trouble than it's worth.

I wanted to suggest it first, even if the code may be larger those are just
stubs while the expensive bug-prone core could exist in a single instance.

Moreover coreaddr is still more effective, at least algorithmically.
(not a concern here as you state.)


> +/* qsort comparison function, that compares mem_ranges.  */

"and sorts them in ascending order according to their START."


> +void
> +normalize_mem_ranges (VEC(mem_range_s) *ranges)

/* This function must not use any VEC operation on RANGES which may reallocate
   the memory block as the callers keep the original memory location.  */


> +{
> +  if (!VEC_empty (mem_range_s, ranges))
> +    {
> +      struct mem_range *ra, *rb;
> +      int a, b;
> +
> +      qsort (VEC_address (mem_range_s, ranges),
> +	     VEC_length (mem_range_s, ranges),
> +	     sizeof (mem_range_s),
> +	     compare_mem_ranges);
> +
> +      a = 0;
> +      ra = VEC_index (mem_range_s, ranges, a);
> +      for (b = 1; VEC_iterate (mem_range_s, ranges, b, rb); b++)
> +	{
> +	  /* If mem_range B overlaps or is adjacent to mem_range A,
> +	     merge them.  */
> +	  if (rb->start <= ra->start + ra->length)
> +	    {
> +	      ra->length = (rb->start + rb->length) - ra->start;
> +	      continue;		/* next b, same a */
> +	    }

Here if `ra->start == rb->start && ra->length > rb->length' this normalization
will lose the `ra->length - rb->length' part.

Not sure if gdbserver can generate such data but at least this function looks
general enough so it should behave general enough.


> +struct mem_range
> +{
> +  /* Lowest address in the range.  */
> +  CORE_ADDR start;
> +
> +  /* Length of the range.  */
> +  int length;
> +};

Why couldn't GDB become 64bit clean - that is CORE_ADDR length.  It is not
supported by the internal gdbserver representation but this structure tries to
be more general.


> +/* Return in RESULT, the set of collected memory in the current
> +   traceframe, found within the LEN bytes range starting at MEMADDR.
> +   Returns true if the target supports the query, otherwise returns
> +   false.  */
            (and RESULTS remains always NULL).
> +
> +int
> +traceframe_available_memory (VEC(mem_range_s) **result,
> +			     CORE_ADDR memaddr, ULONGEST len)

ULONGEST?  It is `int' in `struct mem_range', it should just match the type of
`memaddr'.


> +{
> +  struct traceframe_info *info = get_traceframe_info ();
> +
> +  if (info != NULL)
> +    {
> +      struct mem_range *r;
> +      int i;
> +
> +      *result = NULL;

I would make this line unconditional.  Or the function comment should be
different.


> +      for (i = 0; VEC_iterate (mem_range_s, info->memory, i, r); i++)
> +	if (mem_ranges_overlap (r->start, r->length, memaddr, len))
> +	  {
> +	    ULONGEST lo1, hi1, lo2, hi2;
> +	    struct mem_range *nr;
> +
> +	    lo1 = memaddr;
> +	    hi1 = memaddr + len;
> +
> +	    lo2 = r->start;
> +	    hi2 = r->start + r->length;
> +
> +	    nr = VEC_safe_push (mem_range_s, *result, NULL);
> +
> +	    nr->start = max (lo1, lo2);
> +	    nr->length = min (hi1, hi2) - nr->start;
> +	  }
> +
> +      normalize_mem_ranges (*result);
> +      return 1;
> +    }
> +
> +  return 0;
> +}
> +


Thanks,
Jan


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

* Re: [unavailable values part 1, 03/17] expose list of available ranges to common code
  2011-02-14 11:59 ` Jan Kratochvil
@ 2011-02-14 19:54   ` Pedro Alves
  2011-02-14 21:44     ` Jan Kratochvil
  0 siblings, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2011-02-14 19:54 UTC (permalink / raw)
  To: gdb-patches; +Cc: Jan Kratochvil

On Monday 14 February 2011 11:59:39, Jan Kratochvil wrote:

> > +/* qsort comparison function, that compares mem_ranges.  */
> 
> "and sorts them in ascending order according to their START."

Added something like that.

> > +void
> > +normalize_mem_ranges (VEC(mem_range_s) *ranges)
> 
> /* This function must not use any VEC operation on RANGES which may reallocate
>    the memory block as the callers keep the original memory location.  */

Added something like this too.

> 
> > +{
> > +  if (!VEC_empty (mem_range_s, ranges))
> > +    {
> > +      struct mem_range *ra, *rb;
> > +      int a, b;
> > +
> > +      qsort (VEC_address (mem_range_s, ranges),
> > +	     VEC_length (mem_range_s, ranges),
> > +	     sizeof (mem_range_s),
> > +	     compare_mem_ranges);
> > +
> > +      a = 0;
> > +      ra = VEC_index (mem_range_s, ranges, a);
> > +      for (b = 1; VEC_iterate (mem_range_s, ranges, b, rb); b++)
> > +	{
> > +	  /* If mem_range B overlaps or is adjacent to mem_range A,
> > +	     merge them.  */
> > +	  if (rb->start <= ra->start + ra->length)
> > +	    {
> > +	      ra->length = (rb->start + rb->length) - ra->start;
> > +	      continue;		/* next b, same a */
> > +	    }
> 
> Here if `ra->start == rb->start && ra->length > rb->length' this normalization
> will lose the `ra->length - rb->length' part.

Indeed.  Fixed.

> Not sure if gdbserver can generate such data but at least this function looks
> general enough so it should behave general enough.

Indeed I think it can.

> 
> 
> > +struct mem_range
> > +{
> > +  /* Lowest address in the range.  */
> > +  CORE_ADDR start;
> > +
> > +  /* Length of the range.  */
> > +  int length;
> > +};
> 
> Why couldn't GDB become 64bit clean - that is CORE_ADDR length.  

Probably a leftover from the value ranges stuff (value lengths
are ints, and so I made the value range lengths be ints too).
But I disagree with making it a CORE_ADDR.  I think
lengths should be LONGEST or ULONGEST.  I'll have come back
to this type and length stuff later.

> > +      *result = NULL;
> 
> I would make this line unconditional.  Or the function comment should be
> different.

Tweaked the comment.

Here's what I applied.  Thanks for the review!

Pedro Alves

2011-02-14  Pedro Alves  <pedro@codesourcery.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	gdb/
	* memrange.c (compare_mem_ranges): Mention sort order in
	describing comment.
	(normalize_mem_ranges): Add comment.  Fix ra->length calculation.
	* tracepoint.c (traceframe_available_memory): Extend comment to
	mention what happens to RESULT when the target does not support
	the query.

---
 gdb/memrange.c   |   10 ++++++++--
 gdb/tracepoint.c |    9 +++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

Index: src/gdb/memrange.c
===================================================================
--- src.orig/gdb/memrange.c	2011-02-14 11:19:33.000000000 +0000
+++ src/gdb/memrange.c	2011-02-14 19:04:47.803994993 +0000
@@ -31,7 +31,8 @@ mem_ranges_overlap (CORE_ADDR start1, in
   return (l < h);
 }
 
-/* qsort comparison function, that compares mem_ranges.  */
+/* qsort comparison function, that compares mem_ranges.  Ranges are
+   sorted in ascending START order.  */
 
 static int
 compare_mem_ranges (const void *ap, const void *bp)
@@ -50,6 +51,10 @@ compare_mem_ranges (const void *ap, cons
 void
 normalize_mem_ranges (VEC(mem_range_s) *ranges)
 {
+  /* This function must not use any VEC operation on RANGES that
+     reallocates the memory block as that invalidates the RANGES
+     pointer, which callers expect to remain valid.  */
+
   if (!VEC_empty (mem_range_s, ranges))
     {
       struct mem_range *ra, *rb;
@@ -68,7 +73,8 @@ normalize_mem_ranges (VEC(mem_range_s) *
 	     merge them.  */
 	  if (rb->start <= ra->start + ra->length)
 	    {
-	      ra->length = (rb->start + rb->length) - ra->start;
+	      ra->length = max (ra->length,
+				(rb->start - ra->start) + rb->length);
 	      continue;		/* next b, same a */
 	    }
 	  a++;			/* next a */
Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2011-02-14 12:39:56.000000000 +0000
+++ src/gdb/tracepoint.c	2011-02-14 18:52:32.063995011 +0000
@@ -4635,10 +4635,11 @@ get_traceframe_info (void)
   return traceframe_info;
 }
 
-/* Return in RESULT, the set of collected memory in the current
-   traceframe, found within the LEN bytes range starting at MEMADDR.
-   Returns true if the target supports the query, otherwise returns
-   false.  */
+/* If the target supports the query, return in RESULT the set of
+   collected memory in the current traceframe, found within the LEN
+   bytes range starting at MEMADDR.  Returns true if the target
+   supports the query, otherwise returns false, and RESULT is left
+   undefined.  */
 
 int
 traceframe_available_memory (VEC(mem_range_s) **result,


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

* Re: [unavailable values part 1, 03/17] expose list of available ranges to common code
  2011-02-14 19:54   ` Pedro Alves
@ 2011-02-14 21:44     ` Jan Kratochvil
  2011-02-15 18:42       ` Tom Tromey
  2011-02-15 19:14       ` Pedro Alves
  0 siblings, 2 replies; 7+ messages in thread
From: Jan Kratochvil @ 2011-02-14 21:44 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Mon, 14 Feb 2011 20:39:10 +0100, Pedro Alves wrote:
> On Monday 14 February 2011 11:59:39, Jan Kratochvil wrote:
> > > +struct mem_range
> > > +{
> > > +  /* Lowest address in the range.  */
> > > +  CORE_ADDR start;
> > > +
> > > +  /* Length of the range.  */
> > > +  int length;
> > > +};
> > 
> > Why couldn't GDB become 64bit clean - that is CORE_ADDR length.  
> 
> Probably a leftover from the value ranges stuff (value lengths
> are ints, and so I made the value range lengths be ints too).
> But I disagree with making it a CORE_ADDR.  I think
> lengths should be LONGEST or ULONGEST.

While ULONGEST should work due to this invariant:
	ULONGEST_MAX >= CORE_ADDR_MAX

Still in which case an inferior object size does not fit in CORE_ADDR?
I do not think it can happen, CORE_ADDR should be OK for inferior size_t.


Thanks,
Jan


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

* Re: [unavailable values part 1, 03/17] expose list of available ranges to common code
  2011-02-14 21:44     ` Jan Kratochvil
@ 2011-02-15 18:42       ` Tom Tromey
  2011-02-15 19:14       ` Pedro Alves
  1 sibling, 0 replies; 7+ messages in thread
From: Tom Tromey @ 2011-02-15 18:42 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Pedro Alves, gdb-patches

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Still in which case an inferior object size does not fit in CORE_ADDR?
Jan> I do not think it can happen, CORE_ADDR should be OK for inferior size_t.

We can just make a CORE_SIZE_T and use it.
Then we have some freedom later if we need to typedef it differently.

Tom


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

* Re: [unavailable values part 1, 03/17] expose list of available ranges to common code
  2011-02-14 21:44     ` Jan Kratochvil
  2011-02-15 18:42       ` Tom Tromey
@ 2011-02-15 19:14       ` Pedro Alves
  2011-02-15 21:02         ` Jan Kratochvil
  1 sibling, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2011-02-15 19:14 UTC (permalink / raw)
  To: gdb-patches; +Cc: Jan Kratochvil

On Tuesday 15 February 2011 21:12:23, Jan Kratochvil wrote:
> On Mon, 14 Feb 2011 20:39:10 +0100, Pedro Alves wrote:
> > On Monday 14 February 2011 11:59:39, Jan Kratochvil wrote:
> > > > +struct mem_range
> > > > +{
> > > > +  /* Lowest address in the range.  */
> > > > +  CORE_ADDR start;
> > > > +
> > > > +  /* Length of the range.  */
> > > > +  int length;
> > > > +};
> > > 
> > > Why couldn't GDB become 64bit clean - that is CORE_ADDR length.  
> > 
> > Probably a leftover from the value ranges stuff (value lengths
> > are ints, and so I made the value range lengths be ints too).
> > But I disagree with making it a CORE_ADDR.  I think
> > lengths should be LONGEST or ULONGEST.
> 
> While ULONGEST should work due to this invariant:
> 	ULONGEST_MAX >= CORE_ADDR_MAX
> 
> Still in which case an inferior object size does not fit in CORE_ADDR?
> I do not think it can happen, CORE_ADDR should be OK for inferior size_t.

It's (mainly) not about fitting.  An address is fundamently not the same
as a length/size.  Representing a size with a type meant for
addresses is just not right, IMO.  Imagine if CORE_ADDR was a C++ object
that also included a reference to an address space (I'm not saying
it should, only that it could.  Even today you have the ppc/cell
gdb using special bit hacks in CORE_ADDRs to represent address
spaces).  Then it would be clear that using it for lengths/sizes
wasn't right.  Just as size_t should be used for host sizes, and
uintptr_t/intptr_t/pointers for host addresses. The opposite of what you
said is actually quite possible.  There are systems where the addressable
address space is wider than the size of a single object/array can
be (thus size_t bitwidth may be narrower than than of uintptr_t).

As for what type for use then instead of CORE_ADDR, we traditionaly
use LONGEST/ULONGEST throughout, AFAICS.  I guess nobody ever saw
a real need to come up with a special type for it.

OOC, I did:

 $ grep "CORE_ADDR len" *  -rn
 python/py-inferior.c:63:  CORE_ADDR length;

And I'm going to claim that this only hit shouldn't be
using CORE_ADDR.

Grepping for "LONGEST len" actually finds a large number
of uses.

-- 
Pedro Alves


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

* Re: [unavailable values part 1, 03/17] expose list of available ranges to common code
  2011-02-15 19:14       ` Pedro Alves
@ 2011-02-15 21:02         ` Jan Kratochvil
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Kratochvil @ 2011-02-15 21:02 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Tue, 15 Feb 2011 19:59:03 +0100, Pedro Alves wrote:
> As for what type for use then instead of CORE_ADDR, we traditionaly
> use LONGEST/ULONGEST throughout, AFAICS.  I guess nobody ever saw
> a real need to come up with a special type for it.

OK, I understand and agree with the difference now, thanks for the
explanation.

I will try to introduce some CORE_SIZE_T or similar type when my future
patches need it.


Thanks,
Jan


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

end of thread, other threads:[~2011-02-15 20:44 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-07 14:29 [unavailable values part 1, 03/17] expose list of available ranges to common code Pedro Alves
2011-02-14 11:59 ` Jan Kratochvil
2011-02-14 19:54   ` Pedro Alves
2011-02-14 21:44     ` Jan Kratochvil
2011-02-15 18:42       ` Tom Tromey
2011-02-15 19:14       ` Pedro Alves
2011-02-15 21:02         ` Jan Kratochvil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox