From patchwork Fri Sep 12 20:10:55 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luis Machado X-Patchwork-Id: 2824 Received: (qmail 28166 invoked by alias); 12 Sep 2014 20:11:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28149 invoked by uid 89); 12 Sep 2014 20:11:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_00 autolearn=no version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 12 Sep 2014 20:11:04 +0000 Received: from svr-orw-fem-06.mgc.mentorg.com ([147.34.97.120]) by relay1.mentorg.com with esmtp id 1XSXBM-0001ay-Uc from Luis_Gustavo@mentor.com for gdb-patches@sourceware.org; Fri, 12 Sep 2014 13:11:00 -0700 Received: from [172.30.11.2] (147.34.91.1) by SVR-ORW-FEM-06.mgc.mentorg.com (147.34.97.120) with Microsoft SMTP Server id 14.3.181.6; Fri, 12 Sep 2014 13:11:00 -0700 Message-ID: <5413534F.7000705@codesourcery.com> Date: Fri, 12 Sep 2014 17:10:55 -0300 From: Luis Machado Reply-To: User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: "'gdb-patches@sourceware.org'" Subject: [PATCH] MIPS bit field failures in gdb.base/store.exp X-IsSubscribed: yes Hi, I've been chasing this one for a while, trying to come up with the best solution while not having to do many changes to GDB's type system. On MIPS64 little endian, attempting an assignment to a bit field that lives in a register yields the wrong result. It just corrupts the data in the register depending on the specific position of the bit field inside the structure. FAIL: gdb.base/store.exp: f_1.j FAIL: gdb.base/store.exp: f_1.k FAIL: gdb.base/store.exp: F_1.i FAIL: gdb.base/store.exp: F_1.j FAIL: gdb.base/store.exp: F_1.k FAIL: gdb.base/store.exp: f_2.j FAIL: gdb.base/store.exp: f_2.k FAIL: gdb.base/store.exp: F_2.i FAIL: gdb.base/store.exp: F_2.j FAIL: gdb.base/store.exp: F_2.k FAIL: gdb.base/store.exp: f_3.j FAIL: gdb.base/store.exp: f_3.k FAIL: gdb.base/store.exp: F_3.i FAIL: gdb.base/store.exp: F_3.j FAIL: gdb.base/store.exp: F_3.k FAIL: gdb.base/store.exp: f_4.j FAIL: gdb.base/store.exp: f_4.k FAIL: gdb.base/store.exp: F_4.i FAIL: gdb.base/store.exp: F_4.j FAIL: gdb.base/store.exp: F_4.k === gdb Summary === # of expected passes 220 # of unexpected failures 20 Now, GDB knows how to do bit field assignment properly, but MIPS is one of those architectures that uses a hook for the register-to-value conversion. Although we can properly tell when the type being passed is a structure or union, we cannot tell when it is a bit field, because the bit field data lives in a value structure. Such data only lives in a "type" structure when the parent structure is being referenced, thus you can collect them from the flds_bnds members. A bit field type structure looks pretty much the same as any other primitive type like int or char, so we can't distinguish them. Forcing more fields into the type structure wouldn't help much, because the type structs are shared. It feels to me GDB's type system is a bit dated and needs to be more precise about what it is describing, but for now i just want to fix a pending bug. The most elegant solution i could find without having to touch a number of other type-related data structures is making the gdbarch_convert_register_p predicate accept a value structure instead of a type structure. That way we can properly tell when a bit field is being manipulated in registers. There is still a little problem though. We don't always have a meaningful value struct to pass to this predicate, like both ocurrences of it in findvar.c. In those cases i went for a dummy value. In the end, it is functional but a bit ugly. Unless folks have a better suggestion, is this ok? I did tests with x86, mips32 be/le and mips64 be/le. No regressions found. The lack of bit field data in the type struct also affects other functions that rely on type descriptions, though there may not be explicit failures due to those yet. Luis 2014-09-12 Luis Machado * alpha-tdep.c (alpha_convert_register_p): Exchange struct type parameter for struct value. * arch-utils.c (generic_convert_register_p): Likewise. * arch-utils.h (generic_convert_register_p): Likewise. * findvar.c (value_from_register): Update call to gdbarch_convert_register_p. (address_from_register): Likewise. * gdbarch.c: Regenerate. * gdbarch.h: Regenerate. * gdbarch.sh (convert_register_p): Exchange struct type parameter for struct value. * i386-tdep.c (i386_next_regnum): Likewise. * i387-tdep.c (i387_convert_register_p): Likewise. * i387-tdep.h (i387_convert_register_p): Likewise. * ia64-tdep.c (ia64_convert_register_p): Likewise. * m68k-tdep.c (m68k_convert_register_p): Likewise. * mips-tdep.c (mips_convert_register_p): Likewise. Return 0 for bit field, struct and union values. * rs6000-tdep.c (rs6000_convert_register_p): Exchange struct type parameter for struct value. * valops.c (value_assign): Update call to gdbarch_convert_register_p. * value.c (value_fetch_lazy): Likewise. diff --git a/gdb/alpha-tdep.c b/gdb/alpha-tdep.c index 9e4a8d8..f8d2a2a 100644 --- a/gdb/alpha-tdep.c +++ b/gdb/alpha-tdep.c @@ -230,8 +230,10 @@ alpha_sts (struct gdbarch *gdbarch, void *out, const void *in) static int alpha_convert_register_p (struct gdbarch *gdbarch, int regno, - struct type *type) + struct value *val) { + struct type *type = value_type (val); + return (regno >= ALPHA_FP0_REGNUM && regno < ALPHA_FP0_REGNUM + 31 && TYPE_LENGTH (type) != 8); } diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c index 5ae9fb3..711aad5 100644 --- a/gdb/arch-utils.c +++ b/gdb/arch-utils.c @@ -218,7 +218,7 @@ legacy_virtual_frame_pointer (struct gdbarch *gdbarch, int generic_convert_register_p (struct gdbarch *gdbarch, int regnum, - struct type *type) + struct value *val) { return 0; } diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h index 46d1573..c36516e 100644 --- a/gdb/arch-utils.h +++ b/gdb/arch-utils.h @@ -102,7 +102,7 @@ extern int generic_in_function_epilogue_p (struct gdbarch *gdbarch, /* By default, registers are not convertible. */ extern int generic_convert_register_p (struct gdbarch *gdbarch, int regnum, - struct type *type); + struct value *val); extern int default_stabs_argument_has_addr (struct gdbarch *gdbarch, struct type *type); diff --git a/gdb/findvar.c b/gdb/findvar.c index cb98b86..ae16eee 100644 --- a/gdb/findvar.c +++ b/gdb/findvar.c @@ -701,11 +701,16 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame) struct gdbarch *gdbarch = get_frame_arch (frame); struct type *type1 = check_typedef (type); struct value *v; + struct value *dummy_value = allocate_value_lazy (type); - if (gdbarch_convert_register_p (gdbarch, regnum, type1)) + /* We don't have enough value information at this point, so pass in a dummy + value so the conversion predicate can decide what to do. */ + if (gdbarch_convert_register_p (gdbarch, regnum, dummy_value)) { int optim, unavail, ok; + value_free (dummy_value); + /* The ISA/ABI need to something weird when obtaining the specified value from this register. It might need to re-order non-adjacent, starting with REGNUM (see MIPS and @@ -739,6 +744,7 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame) read_frame_register_value (v, frame); } + value_free (dummy_value); return v; } @@ -751,6 +757,7 @@ address_from_register (int regnum, struct frame_info *frame) struct gdbarch *gdbarch = get_frame_arch (frame); struct type *type = builtin_type (gdbarch)->builtin_data_ptr; struct value *value; + struct value *dummy_value = allocate_value_lazy (type); CORE_ADDR result; /* This routine may be called during early unwinding, at a time @@ -762,11 +769,12 @@ address_from_register (int regnum, struct frame_info *frame) /* Some targets require a special conversion routine even for plain pointer types. Avoid constructing a value object in those cases. */ - if (gdbarch_convert_register_p (gdbarch, regnum, type)) + if (gdbarch_convert_register_p (gdbarch, regnum, dummy_value)) { gdb_byte *buf = alloca (TYPE_LENGTH (type)); int optim, unavail, ok; + value_free (dummy_value); ok = gdbarch_register_to_value (gdbarch, frame, regnum, type, buf, &optim, &unavail); if (!ok) @@ -795,6 +803,7 @@ address_from_register (int regnum, struct frame_info *frame) result = value_as_address (value); release_value (value); + value_free (dummy_value); value_free (value); return result; diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c index b0ee79d..bd97fde 100644 --- a/gdb/gdbarch.c +++ b/gdb/gdbarch.c @@ -2327,13 +2327,13 @@ set_gdbarch_believe_pcc_promotion (struct gdbarch *gdbarch, } int -gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct type *type) +gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct value *val) { gdb_assert (gdbarch != NULL); gdb_assert (gdbarch->convert_register_p != NULL); if (gdbarch_debug >= 2) fprintf_unfiltered (gdb_stdlog, "gdbarch_convert_register_p called\n"); - return gdbarch->convert_register_p (gdbarch, regnum, type); + return gdbarch->convert_register_p (gdbarch, regnum, val); } void diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index 0303b2e..f83a995 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -410,8 +410,8 @@ extern void set_gdbarch_get_longjmp_target (struct gdbarch *gdbarch, gdbarch_get extern int gdbarch_believe_pcc_promotion (struct gdbarch *gdbarch); extern void set_gdbarch_believe_pcc_promotion (struct gdbarch *gdbarch, int believe_pcc_promotion); -typedef int (gdbarch_convert_register_p_ftype) (struct gdbarch *gdbarch, int regnum, struct type *type); -extern int gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct type *type); +typedef int (gdbarch_convert_register_p_ftype) (struct gdbarch *gdbarch, int regnum, struct value *val); +extern int gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct value *val); extern void set_gdbarch_convert_register_p (struct gdbarch *gdbarch, gdbarch_convert_register_p_ftype *convert_register_p); typedef int (gdbarch_register_to_value_ftype) (struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep); diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh index 2a8bca8..8a41f33 100755 --- a/gdb/gdbarch.sh +++ b/gdb/gdbarch.sh @@ -496,7 +496,7 @@ F:int:get_longjmp_target:struct frame_info *frame, CORE_ADDR *pc:frame, pc # v:int:believe_pcc_promotion::::::: # -m:int:convert_register_p:int regnum, struct type *type:regnum, type:0:generic_convert_register_p::0 +m:int:convert_register_p:int regnum, struct value *val:regnum, val:0:generic_convert_register_p::0 f:int:register_to_value:struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep:frame, regnum, type, buf, optimizedp, unavailablep:0 f:void:value_to_register:struct frame_info *frame, int regnum, struct type *type, const gdb_byte *buf:frame, regnum, type, buf:0 # Construct a value representing the contents of register REGNUM in diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index 1e68505..c29e4dd 100644 --- a/gdb/i386-tdep.c +++ b/gdb/i386-tdep.c @@ -3619,8 +3619,9 @@ i386_next_regnum (int regnum) static int i386_convert_register_p (struct gdbarch *gdbarch, - int regnum, struct type *type) + int regnum, struct value *val) { + struct type *type = value_type (val); int len = TYPE_LENGTH (type); /* Values may be spread across multiple registers. Most debugging @@ -3642,7 +3643,7 @@ i386_convert_register_p (struct gdbarch *gdbarch, return 1; } - return i387_convert_register_p (gdbarch, regnum, type); + return i387_convert_register_p (gdbarch, regnum, val); } /* Read a value of type TYPE from register REGNUM in frame FRAME, and diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c index d66ac6a..6e707eb 100644 --- a/gdb/i387-tdep.c +++ b/gdb/i387-tdep.c @@ -336,8 +336,9 @@ i387_print_float_info (struct gdbarch *gdbarch, struct ui_file *file, int i387_convert_register_p (struct gdbarch *gdbarch, int regnum, - struct type *type) + struct value *val) { + struct type *type = value_type (val); if (i386_fp_regnum_p (gdbarch, regnum)) { /* Floating point registers must be converted unless we are diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h index e2eb0f5..4b2c242 100644 --- a/gdb/i387-tdep.h +++ b/gdb/i387-tdep.h @@ -90,7 +90,7 @@ extern void i387_print_float_info (struct gdbarch *gdbarch, needs any special handling. */ extern int i387_convert_register_p (struct gdbarch *gdbarch, int regnum, - struct type *type); + struct value *val); /* Read a value of type TYPE from register REGNUM in frame FRAME, and return its contents in TO. */ diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c index 776a8be..68ceae7 100644 --- a/gdb/ia64-tdep.c +++ b/gdb/ia64-tdep.c @@ -1211,8 +1211,10 @@ ia64_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache, and the special ia64 floating point register format. */ static int -ia64_convert_register_p (struct gdbarch *gdbarch, int regno, struct type *type) +ia64_convert_register_p (struct gdbarch *gdbarch, int regno, struct value *val) { + struct type *type = value_type (val); + return (regno >= IA64_FR0_REGNUM && regno <= IA64_FR127_REGNUM && type != ia64_ext_type (gdbarch)); } diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c index 6bb7e33..94d52e6 100644 --- a/gdb/m68k-tdep.c +++ b/gdb/m68k-tdep.c @@ -188,8 +188,10 @@ m68k_register_name (struct gdbarch *gdbarch, int regnum) static int m68k_convert_register_p (struct gdbarch *gdbarch, - int regnum, struct type *type) + int regnum, struct value *val) { + struct type *type = value_type (val); + if (!gdbarch_tdep (gdbarch)->fpregs_present) return 0; return (regnum >= M68K_FP0_REGNUM && regnum <= M68K_FP0_REGNUM + 7 diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index 188580f..4d20784 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -847,10 +847,23 @@ mips_convert_register_gpreg_case_p (struct gdbarch *gdbarch, int regnum, static int mips_convert_register_p (struct gdbarch *gdbarch, - int regnum, struct type *type) -{ - return (mips_convert_register_float_case_p (gdbarch, regnum, type) - || mips_convert_register_gpreg_case_p (gdbarch, regnum, type)); + int regnum, struct value *val) +{ + int bitfield_or_struct = 0; + struct type *type = check_typedef (value_type (val)); + + /* Check for bitfields, structures or unions. We do not handle + them here. We leave those for generic code to handle. */ + if (value_bitsize (val) != 0 + || TYPE_CODE (type) == TYPE_CODE_STRUCT + || TYPE_CODE (type) == TYPE_CODE_UNION) + bitfield_or_struct = 1; + + return (!bitfield_or_struct + && (mips_convert_register_float_case_p (gdbarch, regnum, + value_type (val)) + || mips_convert_register_gpreg_case_p (gdbarch, regnum, + value_type (val)))); } static int diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c index dabf448..28a5dba 100644 --- a/gdb/rs6000-tdep.c +++ b/gdb/rs6000-tdep.c @@ -2510,9 +2510,10 @@ rs6000_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum, static int rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum, - struct type *type) + struct value *val) { struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); + struct type *type = value_type (val); return (tdep->ppc_fp0_regnum >= 0 && regnum >= tdep->ppc_fp0_regnum diff --git a/gdb/valops.c b/gdb/valops.c index e1decf0..49d36ad 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -1112,7 +1112,7 @@ value_assign (struct value *toval, struct value *fromval) error (_("Value being assigned to is no longer active.")); gdbarch = get_frame_arch (frame); - if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type)) + if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), toval)) { /* If TOVAL is a special machine register requiring conversion of program values to a special raw diff --git a/gdb/value.c b/gdb/value.c index 6620f96..3c2a8da 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -3809,7 +3809,7 @@ value_fetch_lazy (struct value *val) register values should have the register's natural type, so they do not apply. */ gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame), - regnum, type)); + regnum, new_val)); new_val = get_frame_register_value (frame, regnum);