* [PATCH 1/8] More explicit local variable names in ada_value_primitive_packed_val
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
` (2 preceding siblings ...)
2015-10-09 21:42 ` [PATCH 7/8] [Ada] Buffer overflow in ada_unpack_from_contents Joel Brobecker
@ 2015-10-09 21:42 ` Joel Brobecker
2015-10-09 21:42 ` [PATCH 3/8] Reorder variable declarations " Joel Brobecker
` (4 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
A number of local variables declared in ada_value_primitive_packed_val
have a name that could, IMO, be improved to, either: Be more explicit
about what the variable is about (Eg: "src" is an index, so rename it
to "src_idx"); or be more consistent with other variables that they
relate to: for instance, several variables refer to the source via
"src" (Eg: srcBitsLeft, nsrc), but the buffer they refer to is called
"bytes", so patch renames "bytes" to "src".
This should help read and understand a little more easily the code
inside this function. No real code change otherwise.
gdb/ChangeLog:
* ada-lang.c (ada_value_primitive_packed_val): Make the name
of various local variables more explicit and consistent.
No real code change otherwise.
---
gdb/ChangeLog | 6 ++++++
gdb/ada-lang.c | 61 +++++++++++++++++++++++++++++-----------------------------
2 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fe97229..02e9e40 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
+ * ada-lang.c (ada_value_primitive_packed_val): Make the name
+ of various local variables more explicit and consistent.
+ No real code change otherwise.
+
2015-10-09 Simon Marchi <simon.marchi@ericsson.com>
* i386-tdep.h (struct gdbarch_tdep): Change type of
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index a229fa1..6063952 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2398,18 +2398,19 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
struct type *type)
{
struct value *v;
- int src, /* Index into the source area */
- targ, /* Index into the target area */
+ int src_idx, /* Index into the source area */
+ unpacked_idx, /* Index into the unpacked buffer */
srcBitsLeft, /* Number of source bits left to move */
- nsrc, ntarg, /* Number of source and target bytes */
+ src_bytes_left, /* Number of source bytes left to process. */
+ unpacked_bytes_left, /* Number of bytes left to set in unpacked. */
unusedLS, /* Number of bits in next significant
byte of source that are unused */
accumSize; /* Number of meaningful bits in accum */
- unsigned char *bytes; /* First byte containing data to unpack */
+ unsigned char *src; /* First byte containing data to unpack */
unsigned char *unpacked;
unsigned long accum; /* Staging area for bits being transferred */
unsigned char sign;
- int len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
+ int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
/* Transmit bytes from least to most significant; delta is the direction
the indices move. */
int delta = gdbarch_bits_big_endian (get_type_arch (type)) ? -1 : 1;
@@ -2419,7 +2420,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
if (obj == NULL)
{
v = allocate_value (type);
- bytes = (unsigned char *) (valaddr + offset);
+ src = (unsigned char *) valaddr + offset;
}
else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
{
@@ -2434,15 +2435,15 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
less than this stride. In that case, adjust bit_size to
match TYPE's length, and recompute LEN accordingly. */
bit_size = TYPE_LENGTH (type) * HOST_CHAR_BIT;
- len = TYPE_LENGTH (type) + (bit_offset + HOST_CHAR_BIT - 1) / 8;
+ src_len = TYPE_LENGTH (type) + (bit_offset + HOST_CHAR_BIT - 1) / 8;
}
- bytes = (unsigned char *) alloca (len);
- read_memory (value_address (v), bytes, len);
+ src = alloca (src_len);
+ read_memory (value_address (v), src, src_len);
}
else
{
v = allocate_value (type);
- bytes = (unsigned char *) value_contents (obj) + offset;
+ src = (unsigned char *) value_contents (obj) + offset;
}
if (obj != NULL)
@@ -2468,8 +2469,8 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
unpacked = (unsigned char *) value_contents (v);
srcBitsLeft = bit_size;
- nsrc = len;
- ntarg = TYPE_LENGTH (type);
+ src_bytes_left = src_len;
+ unpacked_bytes_left = TYPE_LENGTH (type);
sign = 0;
if (bit_size == 0)
{
@@ -2478,9 +2479,9 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
}
else if (gdbarch_bits_big_endian (get_type_arch (type)))
{
- src = len - 1;
+ src_idx = src_len - 1;
if (has_negatives (type)
- && ((bytes[0] << bit_offset) & (1 << (HOST_CHAR_BIT - 1))))
+ && ((src[0] << bit_offset) & (1 << (HOST_CHAR_BIT - 1))))
sign = ~0;
unusedLS =
@@ -2497,12 +2498,12 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
(HOST_CHAR_BIT - bit_size % HOST_CHAR_BIT) % HOST_CHAR_BIT;
/* ... And are placed at the beginning (most-significant) bytes
of the target. */
- targ = (bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT - 1;
- ntarg = targ + 1;
+ unpacked_idx = (bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT - 1;
+ unpacked_bytes_left = unpacked_idx + 1;
break;
default:
accumSize = 0;
- targ = TYPE_LENGTH (type) - 1;
+ unpacked_idx = TYPE_LENGTH (type) - 1;
break;
}
}
@@ -2510,16 +2511,16 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
{
int sign_bit_offset = (bit_size + bit_offset - 1) % 8;
- src = targ = 0;
+ src_idx = unpacked_idx = 0;
unusedLS = bit_offset;
accumSize = 0;
- if (has_negatives (type) && (bytes[len - 1] & (1 << sign_bit_offset)))
+ if (has_negatives (type) && (src[src_len - 1] & (1 << sign_bit_offset)))
sign = ~0;
}
accum = 0;
- while (nsrc > 0)
+ while (src_bytes_left > 0)
{
/* Mask for removing bits of the next source byte that are not
part of the value. */
@@ -2530,31 +2531,31 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
unsigned int signMask = sign & ~unusedMSMask;
accum |=
- (((bytes[src] >> unusedLS) & unusedMSMask) | signMask) << accumSize;
+ (((src[src_idx] >> unusedLS) & unusedMSMask) | signMask) << accumSize;
accumSize += HOST_CHAR_BIT - unusedLS;
if (accumSize >= HOST_CHAR_BIT)
{
- unpacked[targ] = accum & ~(~0L << HOST_CHAR_BIT);
+ unpacked[unpacked_idx] = accum & ~(~0L << HOST_CHAR_BIT);
accumSize -= HOST_CHAR_BIT;
accum >>= HOST_CHAR_BIT;
- ntarg -= 1;
- targ += delta;
+ unpacked_bytes_left -= 1;
+ unpacked_idx += delta;
}
srcBitsLeft -= HOST_CHAR_BIT - unusedLS;
unusedLS = 0;
- nsrc -= 1;
- src += delta;
+ src_bytes_left -= 1;
+ src_idx += delta;
}
- while (ntarg > 0)
+ while (unpacked_bytes_left > 0)
{
accum |= sign << accumSize;
- unpacked[targ] = accum & ~(~0L << HOST_CHAR_BIT);
+ unpacked[unpacked_idx] = accum & ~(~0L << HOST_CHAR_BIT);
accumSize -= HOST_CHAR_BIT;
if (accumSize < 0)
accumSize = 0;
accum >>= HOST_CHAR_BIT;
- ntarg -= 1;
- targ += delta;
+ unpacked_bytes_left -= 1;
+ unpacked_idx += delta;
}
if (is_dynamic_type (value_type (v)))
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/8] [Ada] Better handling of dynamic types in ada_value_primitive_packed_val
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:42 ` [PATCH 4/8] [Ada] split data unpacking code out of ada_value_primitive_packed_val Joel Brobecker
@ 2015-10-09 21:42 ` Joel Brobecker
2015-10-09 21:42 ` [PATCH 7/8] [Ada] Buffer overflow in ada_unpack_from_contents Joel Brobecker
` (6 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
There is some partial handling for dynamic types in
ada_value_primitive_packed_val, but this support was added
in a fairly ad hoc way, and actually only covered the situation
where OBJ is not NULL and its contents had not been fetched yet.
In addition, even in the cases that it does cover, it doesn't make
much sense. In particular, it was adjusting BIT_SIZE and SRC_LEN,
which are properties of the data to be extracted _from_, based
on TYPE's length once resolved, which is a property of the data
we want to extract _to_.
This patch hopefully adjust this function to handle dynamic types
correctly, and in all cases. It does so by unpacking the data into
a temporary buffer in order to use that buffer to resolve the type.
And _then_ creates the resulting value from that resolved type.
gdb/ChangeLog:
* ada-lang.c (ada_value_primitive_packed_val): Rework handling
of case where TYPE is dynamic.
---
gdb/ChangeLog | 5 ++++
gdb/ada-lang.c | 81 +++++++++++++++++++++++++++++++++++++---------------------
2 files changed, 57 insertions(+), 29 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a511592..06a0637 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
2015-10-09 Joel Brobecker <brobecker@adacore.com>
+ * ada-lang.c (ada_value_primitive_packed_val): Rework handling
+ of case where TYPE is dynamic.
+
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
* ada-lang.c (ada_unpack_from_contents): New function,
extracted from ada_value_primitive_packed_val.
(ada_value_primitive_packed_val): Replace extracted out code
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 6947d76..2b2c47c 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2520,9 +2520,50 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
gdb_byte *unpacked;
int is_scalar;
+ const int is_big_endian = gdbarch_bits_big_endian (get_type_arch (type));
+ gdb_byte *staging = NULL;
+ int staging_len = 0;
+ struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
type = ada_check_typedef (type);
+ switch (TYPE_CODE (type))
+ {
+ case TYPE_CODE_ARRAY:
+ case TYPE_CODE_UNION:
+ case TYPE_CODE_STRUCT:
+ is_scalar = 0;
+ break;
+ default:
+ is_scalar = 1;
+ break;
+ }
+
+ if (obj == NULL)
+ src = (gdb_byte *) valaddr + offset;
+ else
+ src = (gdb_byte *) value_contents (obj) + offset;
+
+ if (is_dynamic_type (type))
+ {
+ /* The length of TYPE might by dynamic, so we need to resolve
+ TYPE in order to know its actual size, which we then use
+ to create the contents buffer of the value we return.
+ The difficulty is that the data containing our object is
+ packed, and therefore maybe not at a byte boundary. So, what
+ we do, is unpack the data into a byte-aligned buffer, and then
+ use that buffer as our object's value for resolving the type. */
+ staging_len = (bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT;
+ staging = malloc (staging_len);
+ make_cleanup (xfree, staging);
+
+ ada_unpack_from_contents (src, bit_offset, bit_size,
+ staging, staging_len,
+ is_big_endian, has_negatives (type),
+ is_scalar);
+ type = resolve_dynamic_type (type, staging, 0);
+ }
+
if (obj == NULL)
{
v = allocate_value (type);
@@ -2531,18 +2572,6 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
{
v = value_at (type, value_address (obj) + offset);
- type = value_type (v);
- if (TYPE_LENGTH (type) * HOST_CHAR_BIT < bit_size)
- {
- /* This can happen in the case of an array of dynamic objects,
- where the size of each element changes from element to element.
- In that case, we're initially given the array stride, but
- after resolving the element type, we find that its size is
- less than this stride. In that case, adjust bit_size to
- match TYPE's length, and recompute LEN accordingly. */
- bit_size = TYPE_LENGTH (type) * HOST_CHAR_BIT;
- src_len = TYPE_LENGTH (type) + (bit_offset + HOST_CHAR_BIT - 1) / 8;
- }
src = alloca (src_len);
read_memory (value_address (v), src, src_len);
}
@@ -2577,29 +2606,23 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
if (bit_size == 0)
{
memset (unpacked, 0, TYPE_LENGTH (type));
+ do_cleanups (old_chain);
return v;
}
- switch (TYPE_CODE (type))
+ if (staging != NULL && staging_len == TYPE_LENGTH (type))
{
- case TYPE_CODE_ARRAY:
- case TYPE_CODE_UNION:
- case TYPE_CODE_STRUCT:
- is_scalar = 0;
- break;
- default:
- is_scalar = 1;
- break;
+ /* Small short-cut: If we've unpacked the data into a buffer
+ of the same size as TYPE's length, then we can reuse that,
+ instead of doing the unpacking again. */
+ memcpy (unpacked, staging, staging_len);
}
+ else
+ ada_unpack_from_contents (src, bit_offset, bit_size,
+ unpacked, TYPE_LENGTH (type),
+ is_big_endian, has_negatives (type), is_scalar);
- ada_unpack_from_contents (src, bit_offset, bit_size,
- unpacked, TYPE_LENGTH (type),
- gdbarch_bits_big_endian (get_type_arch (type)),
- has_negatives (type), is_scalar);
-
- if (is_dynamic_type (value_type (v)))
- v = value_from_contents_and_address (value_type (v), value_contents (v),
- 0);
+ do_cleanups (old_chain);
return v;
}
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 6/8] make is_scalar_type non-static and use it in ada-lang.c
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
` (4 preceding siblings ...)
2015-10-09 21:42 ` [PATCH 3/8] Reorder variable declarations " Joel Brobecker
@ 2015-10-09 21:42 ` Joel Brobecker
2015-10-09 21:42 ` [PATCH 2/8] use gdb_byte in ada-lang.c::ada_value_primitive_packed_val Joel Brobecker
` (2 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
Just a small cleanup, to avoid code duplication...
gdb/ChangeLog:
* gdbtypes.h (is_scalar_type): Add extern declaration.
* gdbtypes.c (is_scalar_type): Make non-static.
* ada-lang.c (ada_value_primitive_packed_val): Use is_scalar_type
to compute IS_SCALAR instead of doing it ourselves.
---
gdb/ChangeLog | 7 +++++++
gdb/ada-lang.c | 14 +-------------
gdb/gdbtypes.c | 2 +-
gdb/gdbtypes.h | 2 ++
4 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 06a0637..9787afe 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
2015-10-09 Joel Brobecker <brobecker@adacore.com>
+ * gdbtypes.h (is_scalar_type): Add extern declaration.
+ * gdbtypes.c (is_scalar_type): Make non-static.
+ * ada-lang.c (ada_value_primitive_packed_val): Use is_scalar_type
+ to compute IS_SCALAR instead of doing it ourselves.
+
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
* ada-lang.c (ada_value_primitive_packed_val): Rework handling
of case where TYPE is dynamic.
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 2b2c47c..d9bbed9 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2519,7 +2519,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
gdb_byte *src; /* First byte containing data to unpack */
int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
gdb_byte *unpacked;
- int is_scalar;
+ const int is_scalar = is_scalar_type (type);
const int is_big_endian = gdbarch_bits_big_endian (get_type_arch (type));
gdb_byte *staging = NULL;
int staging_len = 0;
@@ -2527,18 +2527,6 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
type = ada_check_typedef (type);
- switch (TYPE_CODE (type))
- {
- case TYPE_CODE_ARRAY:
- case TYPE_CODE_UNION:
- case TYPE_CODE_STRUCT:
- is_scalar = 0;
- break;
- default:
- is_scalar = 1;
- break;
- }
-
if (obj == NULL)
src = (gdb_byte *) valaddr + offset;
else
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b406550..919cac9 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2717,7 +2717,7 @@ is_integral_type (struct type *t)
/* Return true if TYPE is scalar. */
-static int
+int
is_scalar_type (struct type *type)
{
type = check_typedef (type);
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 9c64569..0828723 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1912,6 +1912,8 @@ extern int can_dereference (struct type *);
extern int is_integral_type (struct type *);
+extern int is_scalar_type (struct type *type);
+
extern int is_scalar_type_recursive (struct type *);
extern int class_or_union_p (const struct type *);
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 7/8] [Ada] Buffer overflow in ada_unpack_from_contents
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:42 ` [PATCH 4/8] [Ada] split data unpacking code out of ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:42 ` [PATCH 5/8] [Ada] Better handling of dynamic types in ada_value_primitive_packed_val Joel Brobecker
@ 2015-10-09 21:42 ` Joel Brobecker
2015-10-09 21:42 ` [PATCH 1/8] More explicit local variable names in ada_value_primitive_packed_val Joel Brobecker
` (5 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
This patch fixes a buffer overflow in ada_unpack_from_contents
caused by one of the previous commits. This happens when trying
to print the value of an array of variant records.
The overflow happens while trying to print one element of the array.
Because the size of each element in the array is variable, the array
has a DWARF byte_stride attribute, which makes us treat the array
as if it was packed. And during the extraction of each array element,
we try to unpack an object using the array's byte stride as the size,
into an element whose size is actually less than the stride.
This patch fixes the issue by overriding the byte-stride with
the actual element's length.
gdb/ChangeLog:
* ada-lang.c (ada_value_primitive_packed_val): Move
src_len variable to local block where used. Override
BIT_SIZE if bigger than size of resolved type.
---
gdb/ChangeLog | 6 ++++++
gdb/ada-lang.c | 14 +++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9787afe..45e04ae 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
2015-10-09 Joel Brobecker <brobecker@adacore.com>
+ * ada-lang.c (ada_value_primitive_packed_val): Move
+ src_len variable to local block where used. Override
+ BIT_SIZE if bigger than size of resolved type.
+
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
* gdbtypes.h (is_scalar_type): Add extern declaration.
* gdbtypes.c (is_scalar_type): Make non-static.
* ada-lang.c (ada_value_primitive_packed_val): Use is_scalar_type
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index d9bbed9..b7440e2 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2517,7 +2517,6 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
{
struct value *v;
gdb_byte *src; /* First byte containing data to unpack */
- int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
gdb_byte *unpacked;
const int is_scalar = is_scalar_type (type);
const int is_big_endian = gdbarch_bits_big_endian (get_type_arch (type));
@@ -2550,6 +2549,17 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
is_big_endian, has_negatives (type),
is_scalar);
type = resolve_dynamic_type (type, staging, 0);
+ if (TYPE_LENGTH (type) < (bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT)
+ {
+ /* This happens when the length of the object is dynamic,
+ and is actually smaller than the space reserved for it.
+ For instance, in an array of variant records, the bit_size
+ we're given is the array stride, which is constant and
+ normally equal to the maximum size of its element.
+ But, in reality, each element only actually spans a portion
+ of that stride. */
+ bit_size = TYPE_LENGTH (type) * HOST_CHAR_BIT;
+ }
}
if (obj == NULL)
@@ -2559,6 +2569,8 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
}
else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
{
+ int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
+
v = value_at (type, value_address (obj) + offset);
src = alloca (src_len);
read_memory (value_address (v), src, src_len);
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* FYI: [Ada] Rework ada_value_primitive_packed_val
@ 2015-10-09 21:42 Joel Brobecker
2015-10-09 21:42 ` [PATCH 4/8] [Ada] split data unpacking code out of ada_value_primitive_packed_val Joel Brobecker
` (8 more replies)
0 siblings, 9 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
Hello,
FYI, I have just pushed a set of commits we made a while ago at AdaCore
after I realized ada_value_primitive_packed_val could be written in
a way that makes it easier to understand.
Tested on x86_64-linux, no regression. This will start making
a difference when we are able to push some changes we made to
the compiler to produce standard DWARF for various objects (in
this case, for arrays of variant records).
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/8] use gdb_byte in ada-lang.c::ada_value_primitive_packed_val...
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
` (5 preceding siblings ...)
2015-10-09 21:42 ` [PATCH 6/8] make is_scalar_type non-static and use it in ada-lang.c Joel Brobecker
@ 2015-10-09 21:42 ` Joel Brobecker
2015-10-09 21:48 ` [PATCH 8/8] [Ada] ada_unpack_from_contents: Error if target buffer not large enough Joel Brobecker
2015-10-10 18:19 ` FYI: [Ada] Rework ada_value_primitive_packed_val Pedro Alves
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
... instead of "unsigned char".
gdb/Changelog:
* ada-lang.c (ada_value_primitive_packed_val): Change the type
of local variables src and unpacked to "gdb_type *" instead of
"unsigned char *".
---
gdb/ChangeLog | 6 ++++++
gdb/ada-lang.c | 10 +++++-----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 02e9e40..ff3e7d5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
2015-10-09 Joel Brobecker <brobecker@adacore.com>
+ * ada-lang.c (ada_value_primitive_packed_val): Change the type
+ of local variables src and unpacked to "gdb_type *" instead of
+ "unsigned char *".
+
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
* ada-lang.c (ada_value_primitive_packed_val): Make the name
of various local variables more explicit and consistent.
No real code change otherwise.
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 6063952..a5e68d5 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2406,8 +2406,8 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
unusedLS, /* Number of bits in next significant
byte of source that are unused */
accumSize; /* Number of meaningful bits in accum */
- unsigned char *src; /* First byte containing data to unpack */
- unsigned char *unpacked;
+ gdb_byte *src; /* First byte containing data to unpack */
+ gdb_byte *unpacked;
unsigned long accum; /* Staging area for bits being transferred */
unsigned char sign;
int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
@@ -2420,7 +2420,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
if (obj == NULL)
{
v = allocate_value (type);
- src = (unsigned char *) valaddr + offset;
+ src = (gdb_byte *) valaddr + offset;
}
else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
{
@@ -2443,7 +2443,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
else
{
v = allocate_value (type);
- src = (unsigned char *) value_contents (obj) + offset;
+ src = (gdb_byte *) value_contents (obj) + offset;
}
if (obj != NULL)
@@ -2466,7 +2466,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
}
else
set_value_bitsize (v, bit_size);
- unpacked = (unsigned char *) value_contents (v);
+ unpacked = (gdb_byte *) value_contents (v);
srcBitsLeft = bit_size;
src_bytes_left = src_len;
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/8] Reorder variable declarations in ada_value_primitive_packed_val
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
` (3 preceding siblings ...)
2015-10-09 21:42 ` [PATCH 1/8] More explicit local variable names in ada_value_primitive_packed_val Joel Brobecker
@ 2015-10-09 21:42 ` Joel Brobecker
2015-10-09 21:42 ` [PATCH 6/8] make is_scalar_type non-static and use it in ada-lang.c Joel Brobecker
` (3 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
This patch just changes the order in which local variables are declared
so as to group the logically-related variables together. No code
change otherwise.
gdb/ChangeLog:
* ada-lang.c (ada_value_primitive_packed_val): Reorder local
variable declarations.
---
gdb/ChangeLog | 5 +++++
gdb/ada-lang.c | 22 +++++++++++++---------
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ff3e7d5..968992e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
2015-10-09 Joel Brobecker <brobecker@adacore.com>
+ * ada-lang.c (ada_value_primitive_packed_val): Reorder local
+ variable declarations.
+
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
* ada-lang.c (ada_value_primitive_packed_val): Change the type
of local variables src and unpacked to "gdb_type *" instead of
"unsigned char *".
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index a5e68d5..1dbbb07 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2398,19 +2398,23 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
struct type *type)
{
struct value *v;
- int src_idx, /* Index into the source area */
- unpacked_idx, /* Index into the unpacked buffer */
- srcBitsLeft, /* Number of source bits left to move */
- src_bytes_left, /* Number of source bytes left to process. */
- unpacked_bytes_left, /* Number of bytes left to set in unpacked. */
- unusedLS, /* Number of bits in next significant
- byte of source that are unused */
- accumSize; /* Number of meaningful bits in accum */
+
gdb_byte *src; /* First byte containing data to unpack */
+ int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
+ int src_idx; /* Index into the source area */
+ int src_bytes_left; /* Number of source bytes left to process. */
+ int srcBitsLeft; /* Number of source bits left to move */
+ int unusedLS; /* Number of bits in next significant
+ byte of source that are unused */
+
gdb_byte *unpacked;
+ int unpacked_idx; /* Index into the unpacked buffer */
+ int unpacked_bytes_left; /* Number of bytes left to set in unpacked. */
+
unsigned long accum; /* Staging area for bits being transferred */
+ int accumSize; /* Number of meaningful bits in accum */
unsigned char sign;
- int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
+
/* Transmit bytes from least to most significant; delta is the direction
the indices move. */
int delta = gdbarch_bits_big_endian (get_type_arch (type)) ? -1 : 1;
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/8] [Ada] split data unpacking code out of ada_value_primitive_packed_val.
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
@ 2015-10-09 21:42 ` Joel Brobecker
2015-10-09 21:42 ` [PATCH 5/8] [Ada] Better handling of dynamic types in ada_value_primitive_packed_val Joel Brobecker
` (7 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:42 UTC (permalink / raw)
To: gdb-patches
This patch is just preparation work which splits the function
ada_value_primitive_packed_val into two function: one which unpacks
the data, and the other which now uses it to implement
ada_value_primitive_packed_val.
This simplifies a bit ada_value_primitive_packed_val, but will also
allow us to use the new function to unpack data without actually creating
a struct value as a result.
gdb/ChangeLog:
* ada-lang.c (ada_unpack_from_contents): New function,
extracted from ada_value_primitive_packed_val.
(ada_value_primitive_packed_val): Replace extracted out code
by call to ada_unpack_from_contents.
---
gdb/ChangeLog | 7 ++
gdb/ada-lang.c | 217 +++++++++++++++++++++++++++++++++------------------------
2 files changed, 133 insertions(+), 91 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 968992e..a511592 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
2015-10-09 Joel Brobecker <brobecker@adacore.com>
+ * ada-lang.c (ada_unpack_from_contents): New function,
+ extracted from ada_value_primitive_packed_val.
+ (ada_value_primitive_packed_val): Replace extracted out code
+ by call to ada_unpack_from_contents.
+
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
* ada-lang.c (ada_value_primitive_packed_val): Reorder local
variable declarations.
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 1dbbb07..6947d76 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2382,24 +2382,23 @@ has_negatives (struct type *type)
}
}
+/* With SRC being a buffer containing BIT_SIZE bits of data at BIT_OFFSET,
+ unpack that data into UNPACKED. UNPACKED_LEN is the size in bytes of
+ the unpacked buffer.
-/* Create a new value of type TYPE from the contents of OBJ starting
- at byte OFFSET, and bit offset BIT_OFFSET within that byte,
- proceeding for BIT_SIZE bits. If OBJ is an lval in memory, then
- assigning through the result will set the field fetched from.
- VALADDR is ignored unless OBJ is NULL, in which case,
- VALADDR+OFFSET must address the start of storage containing the
- packed value. The value returned in this case is never an lval.
- Assumes 0 <= BIT_OFFSET < HOST_CHAR_BIT. */
+ IS_BIG_ENDIAN is nonzero if the data is stored in big endian mode,
+ zero otherwise.
-struct value *
-ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
- long offset, int bit_offset, int bit_size,
- struct type *type)
-{
- struct value *v;
+ IS_SIGNED_TYPE is nonzero if the data corresponds to a signed type.
- gdb_byte *src; /* First byte containing data to unpack */
+ IS_SCALAR is nonzero if the data corresponds to a signed type. */
+
+static void
+ada_unpack_from_contents (const gdb_byte *src, int bit_offset, int bit_size,
+ gdb_byte *unpacked, int unpacked_len,
+ int is_big_endian, int is_signed_type,
+ int is_scalar)
+{
int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
int src_idx; /* Index into the source area */
int src_bytes_left; /* Number of source bytes left to process. */
@@ -2407,7 +2406,6 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
int unusedLS; /* Number of bits in next significant
byte of source that are unused */
- gdb_byte *unpacked;
int unpacked_idx; /* Index into the unpacked buffer */
int unpacked_bytes_left; /* Number of bytes left to set in unpacked. */
@@ -2417,86 +2415,31 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
/* Transmit bytes from least to most significant; delta is the direction
the indices move. */
- int delta = gdbarch_bits_big_endian (get_type_arch (type)) ? -1 : 1;
-
- type = ada_check_typedef (type);
-
- if (obj == NULL)
- {
- v = allocate_value (type);
- src = (gdb_byte *) valaddr + offset;
- }
- else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
- {
- v = value_at (type, value_address (obj) + offset);
- type = value_type (v);
- if (TYPE_LENGTH (type) * HOST_CHAR_BIT < bit_size)
- {
- /* This can happen in the case of an array of dynamic objects,
- where the size of each element changes from element to element.
- In that case, we're initially given the array stride, but
- after resolving the element type, we find that its size is
- less than this stride. In that case, adjust bit_size to
- match TYPE's length, and recompute LEN accordingly. */
- bit_size = TYPE_LENGTH (type) * HOST_CHAR_BIT;
- src_len = TYPE_LENGTH (type) + (bit_offset + HOST_CHAR_BIT - 1) / 8;
- }
- src = alloca (src_len);
- read_memory (value_address (v), src, src_len);
- }
- else
- {
- v = allocate_value (type);
- src = (gdb_byte *) value_contents (obj) + offset;
- }
-
- if (obj != NULL)
- {
- long new_offset = offset;
-
- set_value_component_location (v, obj);
- set_value_bitpos (v, bit_offset + value_bitpos (obj));
- set_value_bitsize (v, bit_size);
- if (value_bitpos (v) >= HOST_CHAR_BIT)
- {
- ++new_offset;
- set_value_bitpos (v, value_bitpos (v) - HOST_CHAR_BIT);
- }
- set_value_offset (v, new_offset);
-
- /* Also set the parent value. This is needed when trying to
- assign a new value (in inferior memory). */
- set_value_parent (v, obj);
- }
- else
- set_value_bitsize (v, bit_size);
- unpacked = (gdb_byte *) value_contents (v);
+ int delta = is_big_endian ? -1 : 1;
srcBitsLeft = bit_size;
src_bytes_left = src_len;
- unpacked_bytes_left = TYPE_LENGTH (type);
+ unpacked_bytes_left = unpacked_len;
sign = 0;
- if (bit_size == 0)
- {
- memset (unpacked, 0, TYPE_LENGTH (type));
- return v;
- }
- else if (gdbarch_bits_big_endian (get_type_arch (type)))
+
+ if (is_big_endian)
{
src_idx = src_len - 1;
- if (has_negatives (type)
- && ((src[0] << bit_offset) & (1 << (HOST_CHAR_BIT - 1))))
+ if (is_signed_type
+ && ((src[0] << bit_offset) & (1 << (HOST_CHAR_BIT - 1))))
sign = ~0;
unusedLS =
(HOST_CHAR_BIT - (bit_size + bit_offset) % HOST_CHAR_BIT)
% HOST_CHAR_BIT;
- switch (TYPE_CODE (type))
- {
- case TYPE_CODE_ARRAY:
- case TYPE_CODE_UNION:
- case TYPE_CODE_STRUCT:
+ if (is_scalar)
+ {
+ accumSize = 0;
+ unpacked_idx = unpacked_len - 1;
+ }
+ else
+ {
/* Non-scalar values must be aligned at a byte boundary... */
accumSize =
(HOST_CHAR_BIT - bit_size % HOST_CHAR_BIT) % HOST_CHAR_BIT;
@@ -2504,12 +2447,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
of the target. */
unpacked_idx = (bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT - 1;
unpacked_bytes_left = unpacked_idx + 1;
- break;
- default:
- accumSize = 0;
- unpacked_idx = TYPE_LENGTH (type) - 1;
- break;
- }
+ }
}
else
{
@@ -2519,7 +2457,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
unusedLS = bit_offset;
accumSize = 0;
- if (has_negatives (type) && (src[src_len - 1] & (1 << sign_bit_offset)))
+ if (is_signed_type && (src[src_len - 1] & (1 << sign_bit_offset)))
sign = ~0;
}
@@ -2561,6 +2499,103 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
unpacked_bytes_left -= 1;
unpacked_idx += delta;
}
+}
+
+/* Create a new value of type TYPE from the contents of OBJ starting
+ at byte OFFSET, and bit offset BIT_OFFSET within that byte,
+ proceeding for BIT_SIZE bits. If OBJ is an lval in memory, then
+ assigning through the result will set the field fetched from.
+ VALADDR is ignored unless OBJ is NULL, in which case,
+ VALADDR+OFFSET must address the start of storage containing the
+ packed value. The value returned in this case is never an lval.
+ Assumes 0 <= BIT_OFFSET < HOST_CHAR_BIT. */
+
+struct value *
+ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
+ long offset, int bit_offset, int bit_size,
+ struct type *type)
+{
+ struct value *v;
+ gdb_byte *src; /* First byte containing data to unpack */
+ int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
+ gdb_byte *unpacked;
+ int is_scalar;
+
+ type = ada_check_typedef (type);
+
+ if (obj == NULL)
+ {
+ v = allocate_value (type);
+ src = (gdb_byte *) valaddr + offset;
+ }
+ else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
+ {
+ v = value_at (type, value_address (obj) + offset);
+ type = value_type (v);
+ if (TYPE_LENGTH (type) * HOST_CHAR_BIT < bit_size)
+ {
+ /* This can happen in the case of an array of dynamic objects,
+ where the size of each element changes from element to element.
+ In that case, we're initially given the array stride, but
+ after resolving the element type, we find that its size is
+ less than this stride. In that case, adjust bit_size to
+ match TYPE's length, and recompute LEN accordingly. */
+ bit_size = TYPE_LENGTH (type) * HOST_CHAR_BIT;
+ src_len = TYPE_LENGTH (type) + (bit_offset + HOST_CHAR_BIT - 1) / 8;
+ }
+ src = alloca (src_len);
+ read_memory (value_address (v), src, src_len);
+ }
+ else
+ {
+ v = allocate_value (type);
+ src = (gdb_byte *) value_contents (obj) + offset;
+ }
+
+ if (obj != NULL)
+ {
+ long new_offset = offset;
+
+ set_value_component_location (v, obj);
+ set_value_bitpos (v, bit_offset + value_bitpos (obj));
+ set_value_bitsize (v, bit_size);
+ if (value_bitpos (v) >= HOST_CHAR_BIT)
+ {
+ ++new_offset;
+ set_value_bitpos (v, value_bitpos (v) - HOST_CHAR_BIT);
+ }
+ set_value_offset (v, new_offset);
+
+ /* Also set the parent value. This is needed when trying to
+ assign a new value (in inferior memory). */
+ set_value_parent (v, obj);
+ }
+ else
+ set_value_bitsize (v, bit_size);
+ unpacked = (gdb_byte *) value_contents (v);
+
+ if (bit_size == 0)
+ {
+ memset (unpacked, 0, TYPE_LENGTH (type));
+ return v;
+ }
+
+ switch (TYPE_CODE (type))
+ {
+ case TYPE_CODE_ARRAY:
+ case TYPE_CODE_UNION:
+ case TYPE_CODE_STRUCT:
+ is_scalar = 0;
+ break;
+ default:
+ is_scalar = 1;
+ break;
+ }
+
+ ada_unpack_from_contents (src, bit_offset, bit_size,
+ unpacked, TYPE_LENGTH (type),
+ gdbarch_bits_big_endian (get_type_arch (type)),
+ has_negatives (type), is_scalar);
if (is_dynamic_type (value_type (v)))
v = value_from_contents_and_address (value_type (v), value_contents (v),
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 8/8] [Ada] ada_unpack_from_contents: Error if target buffer not large enough
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
` (6 preceding siblings ...)
2015-10-09 21:42 ` [PATCH 2/8] use gdb_byte in ada-lang.c::ada_value_primitive_packed_val Joel Brobecker
@ 2015-10-09 21:48 ` Joel Brobecker
2015-10-10 18:19 ` FYI: [Ada] Rework ada_value_primitive_packed_val Pedro Alves
8 siblings, 0 replies; 12+ messages in thread
From: Joel Brobecker @ 2015-10-09 21:48 UTC (permalink / raw)
To: gdb-patches
This adds a guard that the size of the "unpacked" buffer is large enough
to contain at least BIT_SIZE bits. If not, report an error. This is to
guard this routine from doing buffer overflows when called incorrectly.
gdb/ChangeLog:
* ada-lang.c (ada_unpack_from_contents): Add guard that unpacked
is large enough for BIT_SIZE. Update function comment.
---
gdb/ChangeLog | 5 +++++
gdb/ada-lang.c | 11 ++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 45e04ae..578aeb8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
2015-10-09 Joel Brobecker <brobecker@adacore.com>
+ * ada-lang.c (ada_unpack_from_contents): Add guard that unpacked
+ is large enough for BIT_SIZE. Update function comment.
+
+2015-10-09 Joel Brobecker <brobecker@adacore.com>
+
* ada-lang.c (ada_value_primitive_packed_val): Move
src_len variable to local block where used. Override
BIT_SIZE if bigger than size of resolved type.
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index b7440e2..97f0c49 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2383,9 +2383,12 @@ has_negatives (struct type *type)
}
/* With SRC being a buffer containing BIT_SIZE bits of data at BIT_OFFSET,
- unpack that data into UNPACKED. UNPACKED_LEN is the size in bytes of
+ unpack that data into UNPACKED. UNPACKED_LEN is the size in bytes of
the unpacked buffer.
+ The size of the unpacked buffer (UNPACKED_LEN) is expected to be large
+ enough to contain at least BIT_OFFSET bits. If not, an error is raised.
+
IS_BIG_ENDIAN is nonzero if the data is stored in big endian mode,
zero otherwise.
@@ -2417,6 +2420,12 @@ ada_unpack_from_contents (const gdb_byte *src, int bit_offset, int bit_size,
the indices move. */
int delta = is_big_endian ? -1 : 1;
+ /* Make sure that unpacked is large enough to receive the BIT_SIZE
+ bits from SRC. .*/
+ if ((bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT > unpacked_len)
+ error (_("Cannot unpack %d bits into buffer of %d bytes"),
+ bit_size, unpacked_len);
+
srcBitsLeft = bit_size;
src_bytes_left = src_len;
unpacked_bytes_left = unpacked_len;
--
2.1.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FYI: [Ada] Rework ada_value_primitive_packed_val
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
` (7 preceding siblings ...)
2015-10-09 21:48 ` [PATCH 8/8] [Ada] ada_unpack_from_contents: Error if target buffer not large enough Joel Brobecker
@ 2015-10-10 18:19 ` Pedro Alves
2015-10-12 15:56 ` Joel Brobecker
8 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2015-10-10 18:19 UTC (permalink / raw)
To: Joel Brobecker, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 797 bytes --]
Hi Joel,
On 10/09/2015 10:41 PM, Joel Brobecker wrote:
> FYI, I have just pushed a set of commits we made a while ago at AdaCore
> after I realized ada_value_primitive_packed_val could be written in
> a way that makes it easier to understand.
I noticed that this series regressed the C++ build:
../../src/gdb/ada-lang.c: In function ‘value* ada_value_primitive_packed_val(value*, const gdb_byte*, long int, int, int, type*)’:
../../src/gdb/ada-lang.c:2553:36: error: invalid conversion from ‘void*’ to ‘gdb_byte* {aka unsigned char*}’ [-fpermissive]
staging = malloc (staging_len);
^
(similarly for alloca.)
Looking at the code, I noticed the newly added casts, and thought of giving
it a try at eliminating them. See patches below. WDYT?
[-- Attachment #2: 0001-ada-lang.c-malloc-alloca-casts-for-C.patch --]
[-- Type: text/x-patch, Size: 1429 bytes --]
From b58eef432cc9705a3e95d2cf91b0f64ae102f2e1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 10 Oct 2015 14:31:41 +0100
Subject: [PATCH 1/2] ada-lang.c: malloc/alloca casts for C++
gdb/ChangeLog:
2015-10-10 Pedro Alves <palves@redhat.com>
* ada-lang.c (ada_value_primitive_packed_val): Add casts to malloc
and alloca calls.
---
gdb/ada-lang.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 97f0c49..6d1998a 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2550,7 +2550,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
we do, is unpack the data into a byte-aligned buffer, and then
use that buffer as our object's value for resolving the type. */
staging_len = (bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT;
- staging = malloc (staging_len);
+ staging = (gdb_byte *) malloc (staging_len);
make_cleanup (xfree, staging);
ada_unpack_from_contents (src, bit_offset, bit_size,
@@ -2581,7 +2581,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
v = value_at (type, value_address (obj) + offset);
- src = alloca (src_len);
+ src = (gdb_byte *) alloca (src_len);
read_memory (value_address (v), src, src_len);
}
else
--
1.9.3
[-- Attachment #3: 0002-ada-lang.c-ada_value_primitive_packed_val-const-corr.patch --]
[-- Type: text/x-patch, Size: 2618 bytes --]
From be61a6e02fc3dce50f4ce16faa3b8a458d2a451a Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sat, 10 Oct 2015 13:53:47 +0100
Subject: [PATCH 2/2] ada-lang.c:ada_value_primitive_packed_val: const
correctness
gdb/ChangeLog:
2015-10-10 Pedro Alves <palves@redhat.com>
* ada-lang.c (ada_value_primitive_packed_val): Constify
locals. Use value_contents_writeable. Remove casts.
---
gdb/ada-lang.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 6d1998a..383db99 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2525,7 +2525,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
struct type *type)
{
struct value *v;
- gdb_byte *src; /* First byte containing data to unpack */
+ const gdb_byte *src; /* First byte containing data to unpack */
gdb_byte *unpacked;
const int is_scalar = is_scalar_type (type);
const int is_big_endian = gdbarch_bits_big_endian (get_type_arch (type));
@@ -2536,9 +2536,9 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
type = ada_check_typedef (type);
if (obj == NULL)
- src = (gdb_byte *) valaddr + offset;
+ src = valaddr + offset;
else
- src = (gdb_byte *) value_contents (obj) + offset;
+ src = value_contents (obj) + offset;
if (is_dynamic_type (type))
{
@@ -2574,20 +2574,22 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
if (obj == NULL)
{
v = allocate_value (type);
- src = (gdb_byte *) valaddr + offset;
+ src = valaddr + offset;
}
else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
{
int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
+ gdb_byte *buf;
v = value_at (type, value_address (obj) + offset);
- src = (gdb_byte *) alloca (src_len);
- read_memory (value_address (v), src, src_len);
+ buf = (gdb_byte *) alloca (src_len);
+ read_memory (value_address (v), buf, src_len);
+ src = buf;
}
else
{
v = allocate_value (type);
- src = (gdb_byte *) value_contents (obj) + offset;
+ src = value_contents (obj) + offset;
}
if (obj != NULL)
@@ -2610,7 +2612,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
}
else
set_value_bitsize (v, bit_size);
- unpacked = (gdb_byte *) value_contents (v);
+ unpacked = value_contents_writeable (v);
if (bit_size == 0)
{
--
1.9.3
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FYI: [Ada] Rework ada_value_primitive_packed_val
2015-10-10 18:19 ` FYI: [Ada] Rework ada_value_primitive_packed_val Pedro Alves
@ 2015-10-12 15:56 ` Joel Brobecker
2015-10-13 18:42 ` Pedro Alves
0 siblings, 1 reply; 12+ messages in thread
From: Joel Brobecker @ 2015-10-12 15:56 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
> Looking at the code, I noticed the newly added casts, and thought of giving
> it a try at eliminating them. See patches below. WDYT?
They look good, thanks for doing that!
> >From b58eef432cc9705a3e95d2cf91b0f64ae102f2e1 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sat, 10 Oct 2015 14:31:41 +0100
> Subject: [PATCH 1/2] ada-lang.c: malloc/alloca casts for C++
>
> gdb/ChangeLog:
> 2015-10-10 Pedro Alves <palves@redhat.com>
>
> * ada-lang.c (ada_value_primitive_packed_val): Add casts to malloc
> and alloca calls.
> ---
> gdb/ada-lang.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 97f0c49..6d1998a 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -2550,7 +2550,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
> we do, is unpack the data into a byte-aligned buffer, and then
> use that buffer as our object's value for resolving the type. */
> staging_len = (bit_size + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT;
> - staging = malloc (staging_len);
> + staging = (gdb_byte *) malloc (staging_len);
> make_cleanup (xfree, staging);
>
> ada_unpack_from_contents (src, bit_offset, bit_size,
> @@ -2581,7 +2581,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
> int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
>
> v = value_at (type, value_address (obj) + offset);
> - src = alloca (src_len);
> + src = (gdb_byte *) alloca (src_len);
> read_memory (value_address (v), src, src_len);
> }
> else
> --
> 1.9.3
>
> >From be61a6e02fc3dce50f4ce16faa3b8a458d2a451a Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sat, 10 Oct 2015 13:53:47 +0100
> Subject: [PATCH 2/2] ada-lang.c:ada_value_primitive_packed_val: const
> correctness
>
> gdb/ChangeLog:
> 2015-10-10 Pedro Alves <palves@redhat.com>
>
> * ada-lang.c (ada_value_primitive_packed_val): Constify
> locals. Use value_contents_writeable. Remove casts.
> ---
> gdb/ada-lang.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 6d1998a..383db99 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -2525,7 +2525,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
> struct type *type)
> {
> struct value *v;
> - gdb_byte *src; /* First byte containing data to unpack */
> + const gdb_byte *src; /* First byte containing data to unpack */
> gdb_byte *unpacked;
> const int is_scalar = is_scalar_type (type);
> const int is_big_endian = gdbarch_bits_big_endian (get_type_arch (type));
> @@ -2536,9 +2536,9 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
> type = ada_check_typedef (type);
>
> if (obj == NULL)
> - src = (gdb_byte *) valaddr + offset;
> + src = valaddr + offset;
> else
> - src = (gdb_byte *) value_contents (obj) + offset;
> + src = value_contents (obj) + offset;
>
> if (is_dynamic_type (type))
> {
> @@ -2574,20 +2574,22 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
> if (obj == NULL)
> {
> v = allocate_value (type);
> - src = (gdb_byte *) valaddr + offset;
> + src = valaddr + offset;
> }
> else if (VALUE_LVAL (obj) == lval_memory && value_lazy (obj))
> {
> int src_len = (bit_size + bit_offset + HOST_CHAR_BIT - 1) / 8;
> + gdb_byte *buf;
>
> v = value_at (type, value_address (obj) + offset);
> - src = (gdb_byte *) alloca (src_len);
> - read_memory (value_address (v), src, src_len);
> + buf = (gdb_byte *) alloca (src_len);
> + read_memory (value_address (v), buf, src_len);
> + src = buf;
> }
> else
> {
> v = allocate_value (type);
> - src = (gdb_byte *) value_contents (obj) + offset;
> + src = value_contents (obj) + offset;
> }
>
> if (obj != NULL)
> @@ -2610,7 +2612,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
> }
> else
> set_value_bitsize (v, bit_size);
> - unpacked = (gdb_byte *) value_contents (v);
> + unpacked = value_contents_writeable (v);
>
> if (bit_size == 0)
> {
> --
> 1.9.3
>
--
Joel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: FYI: [Ada] Rework ada_value_primitive_packed_val
2015-10-12 15:56 ` Joel Brobecker
@ 2015-10-13 18:42 ` Pedro Alves
0 siblings, 0 replies; 12+ messages in thread
From: Pedro Alves @ 2015-10-13 18:42 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches
On 10/12/2015 04:56 PM, Joel Brobecker wrote:
>> Looking at the code, I noticed the newly added casts, and thought of giving
>> it a try at eliminating them. See patches below. WDYT?
>
> They look good, thanks for doing that!
Pushed, thanks.
--
Pedro Alves
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-10-13 18:42 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-09 21:42 FYI: [Ada] Rework ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:42 ` [PATCH 4/8] [Ada] split data unpacking code out of ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:42 ` [PATCH 5/8] [Ada] Better handling of dynamic types in ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:42 ` [PATCH 7/8] [Ada] Buffer overflow in ada_unpack_from_contents Joel Brobecker
2015-10-09 21:42 ` [PATCH 1/8] More explicit local variable names in ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:42 ` [PATCH 3/8] Reorder variable declarations " Joel Brobecker
2015-10-09 21:42 ` [PATCH 6/8] make is_scalar_type non-static and use it in ada-lang.c Joel Brobecker
2015-10-09 21:42 ` [PATCH 2/8] use gdb_byte in ada-lang.c::ada_value_primitive_packed_val Joel Brobecker
2015-10-09 21:48 ` [PATCH 8/8] [Ada] ada_unpack_from_contents: Error if target buffer not large enough Joel Brobecker
2015-10-10 18:19 ` FYI: [Ada] Rework ada_value_primitive_packed_val Pedro Alves
2015-10-12 15:56 ` Joel Brobecker
2015-10-13 18:42 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).