From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22112 invoked by alias); 14 May 2010 11:05:54 -0000 Received: (qmail 22091 invoked by uid 22791); 14 May 2010 11:05:51 -0000 X-SWARE-Spam-Status: No, hits=-5.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 14 May 2010 11:05:42 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o4EB5fqJ019148 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 14 May 2010 07:05:41 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o4EB5cHU030056 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 14 May 2010 07:05:40 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o4EB5cu3002008; Fri, 14 May 2010 13:05:38 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o4EB5bpt002000; Fri, 14 May 2010 13:05:37 +0200 Date: Fri, 14 May 2010 12:20:00 -0000 From: Jan Kratochvil To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: RFC: fix bug in pieced value with offset Message-ID: <20100514110537.GA25586@host0.dyn.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-08-17) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-05/txt/msg00298.txt.bz2 On Thu, 13 May 2010 19:04:55 +0200, Tom Tromey wrote: > --- dwarf2loc.c 13 May 2010 15:44:35 -0000 1.77 > +++ dwarf2loc.c 13 May 2010 17:01:01 -0000 # @@ -277,17 +306,18 @@ read_pieced_value (struct value *v) # case DWARF_VALUE_REGISTER: > struct gdbarch *arch = get_frame_arch (frame); > int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, > p->v.expr.value); > - int reg_offset = 0; > + int reg_offset = source_offset; > > if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG > - && p->size < register_size (arch, gdb_regnum)) > + && this_size + reg_offset <= register_size (arch, gdb_regnum)) > /* Big-endian, and we want less than full size. */ > - reg_offset = register_size (arch, gdb_regnum) - p->size; > + reg_offset = (register_size (arch, gdb_regnum) > + - this_size - reg_offset); Thinking now if the BFD_ENDIAN_BIG patch by Ulrich Weigand [rfc] Handle DWARF-2 value pieces residing in *parts* of a register http://sourceware.org/ml/gdb-patches/2009-12/msg00305.html should not have been applied also for DWARF_VALUE_STACK; but this is outside of the scope of this patch. I think here is missing some size/validity warning()/error() as discussed for DWARF_VALUE_STACK below. In such case the whole condition could be removed: > - && p->size < register_size (arch, gdb_regnum)) > + && this_size + reg_offset <= register_size (arch, gdb_regnum)) I believe it should be instead: # + reg_offset = (register_size (arch, gdb_regnum) # + - this_size); ^^^ As we should ignore source_offset bytes from the start of register. register_size = 8 p->size = 4 bytes_to_skip = for example 1 => source_offset = 1 this_size = 3 >From the register occupying bytes <0..7> we thus want to read-in bytes <5..7>. (Currently we assume we always want to read up to infinity as you noted in the followup mail: http://sourceware.org/ml/gdb-patches/2010-05/msg00282.html In reality we may want to read less than up to <..7>.) > case DWARF_VALUE_STACK: > { > struct gdbarch *gdbarch = get_type_arch (value_type (v)); > - size_t n = p->size; > + size_t n = this_size; > if (n > c->addr_size) > n = c->addr_size; Generally I would prefer more sanity checks there instead of quiet data cutting. Moreover for example get_frame_register_bytes() currently performs non-fatal warning: { warning (_("Bad debug information detected: " "Attempt to read %d bytes from registers."), len); return 0; } while IMO in these cases it could error(). There is also missing `- source_offset': # if (n > c->addr_size - source_offset) # n = c->addr_size - source_offset; > - store_unsigned_integer (contents + offset, n, > - gdbarch_byte_order (gdbarch), > - p->v.expr.value); > + if (source_offset == 0) > + store_unsigned_integer (contents + dest_offset, n, > + gdbarch_byte_order (gdbarch), > + p->v.expr.value); > + else > + { > + gdb_byte bytes[sizeof (ULONGEST)]; Missing an empty line for declarations separation. > + store_unsigned_integer (bytes, n, Instead of `n' there should be `n + source_offset'. > + gdbarch_byte_order (gdbarch), > + p->v.expr.value); > + memcpy (contents + dest_offset, bytes + source_offset, n); > + } > } > break; > > case DWARF_VALUE_LITERAL: > { > - size_t n = p->size; > - if (n > p->v.literal.length) > - n = p->v.literal.length; > - memcpy (contents + offset, p->v.literal.data, n); > + if (this_size > p->v.literal.length) > + this_size = p->v.literal.length; Again missing `- source_offset' and it should be IMO more some error(): # + if (this_size > p->v.literal.length - source_offset) # + this_size = p->v.literal.length - source_offset; # @@ -347,27 +389,51 @@ write_pieced_value (struct value *to, struct value *from) [...] > switch (p->location) > { > case DWARF_VALUE_REGISTER: > { > struct gdbarch *arch = get_frame_arch (frame); > int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.expr.value); > - int reg_offset = 0; > + int reg_offset = dest_offset; > as in read_pieced_value: Missing offset/size validation. > if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG > - && p->size < register_size (arch, gdb_regnum)) > + && this_size + reg_offset <= register_size (arch, gdb_regnum)) as in read_pieced_value: The conditional could be probably removed. > /* Big-endian, and we want less than full size. */ > - reg_offset = register_size (arch, gdb_regnum) - p->size; > + reg_offset = (register_size (arch, gdb_regnum) - this_size > + - reg_offset); as in read_pieced_value: Excessive `- reg_offset'. > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ testsuite/gdb.dwarf2/pieces.S 13 May 2010 17:01:03 -0000 [...] > +.globl _start > + .type _start, @function > +_start: Together with: > + [list {additional_flags=-nostdlib}]] != "" } { Why weren't just simple "main" and standard compilation used? It works for me. > Index: testsuite/gdb.dwarf2/pieces.c > =================================================================== > RCS file: testsuite/gdb.dwarf2/pieces.c > diff -N testsuite/gdb.dwarf2/pieces.c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ testsuite/gdb.dwarf2/pieces.c 13 May 2010 17:01:03 -0000 > @@ -0,0 +1,80 @@ > +/* The original program corresponding to pieces.c. > + This originally came from https://bugzilla.redhat.com/show_bug.cgi?id=589467 > + Note that it is not ever compiled, pieces.S is used instead. */ Isn't missing the ubiquitous FSF copyleft header? > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ testsuite/gdb.dwarf2/pieces.exp 13 May 2010 17:01:03 -0000 > @@ -0,0 +1,60 @@ [...] > +set testfile "pieces" > +set srcfile ${testfile}.S > +set binfile ${objdir}/${subdir}/${testfile}.x > + > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \ > + [list {additional_flags=-nostdlib}]] != "" } { > + return -1 > +} If -nostdlib would get removed by using "main" then we could prepare_for_testing. > + > +gdb_exit > +gdb_start > +gdb_reinitialize_dir $srcdir/$subdir > +gdb_load ${binfile} clean_restart ${testfile}.x > +# Function f1 tests a particular gdb bug involving DW_OP_piece. > +proc pieces_test_f1 {} { > + gdb_test "break pieces.c:22" "Breakpoint 2.*" \ > + "set first breakpoint for pieces" > + gdb_continue_to_breakpoint "first continue to breakpoint for pieces" I would use also the second parameter to verify the stop point but it is a bit controversial whether to use it: # gdb_continue_to_breakpoint "first continue to breakpoint for pieces" ".*pieces.c:22\r\n.*" Thanks, Jan