From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 4D4033857016 for ; Mon, 13 Nov 2023 15:00:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4D4033857016 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4D4033857016 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699887652; cv=none; b=RuC8w7Qw2AlxQLOA6etgSYxa2l75XZs3ZHAs0ljRHBifXWQWcH8rcb2+6tV1Zdk666tdvASTigsSmc37pPit6VCijz5TDj9sv8HHYxH/Jwb3C24IJlVKtmdtu+aZqSFZta8xDYf5lowDFN6uLknvSjVCKrB7Sf9Dc7G1AafNS5Y= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699887652; c=relaxed/simple; bh=mFyHn/Z4fXVT9EmkaC6sNU8aeYMoYlHsMoUHuOe1rQU=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=aGgvkjbHk9m819qIBxCEMu+HWB+YWs4EIO9nmnyFu7+kc9wfYjDy7dZSK2FQ88fgTL9PnBGi8o1b2Lv1rHOi6H7vQBkE96728ie3JnMeJRyCNG6JKrgwk9bdVM0qmzyiZ0pCaanqQ8QZSDaDQXW06ffTJi3gPgNV4zKTa1AP6PI= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699887649; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=e1MPDIC8mjC6W9+t3jGJa4YEtYKuMokakVwSgSDFLxs=; b=G5WiXxZp9BdvC2LNeJHTAi8tHDUXqvArQuS6fK7LfglLAF+n4hXk0Y3BiDHmBnHl3LMzVi HCG9rq7467TF/xb4WZ3Z1CkFXE90RdePfXEDaY7eHMBi+rRjdjqVP873Ih2BQjxnaICerV REhoFeS202zpsy0L6iXuEv33CXyUwTM= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-681-cfcwPSbgNs25KWdwXvXhuw-1; Mon, 13 Nov 2023 10:00:48 -0500 X-MC-Unique: cfcwPSbgNs25KWdwXvXhuw-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-40839252e81so28869115e9.3 for ; Mon, 13 Nov 2023 07:00:48 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699887647; x=1700492447; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=e1MPDIC8mjC6W9+t3jGJa4YEtYKuMokakVwSgSDFLxs=; b=WAHjLqlaOxMvmDs9flQoTe8CfGlq7WoC8xBhoYHgV07mDzAAbeBSkEArsPOhHbRb5E orUHhrs5bBO8U1fcgWLCbLiBsqlfxT8YSxd8lTr7vCjp5Jz2n2j9WkdQ7ykcJY3FDD0s 9TT0+5aqRCHrE9mfd8jgVNiW7Tp6rqMBAAMLZu9JOhqqjFvci896O/uuHs4fwS0tVh0M 1rEebT2JLflfddVTbPn491q1WQ/9jS9uS0wPVH0jchn6sYHsG0cmZRrnwRtyR8tAtqEl V+ljB2pnY+eqBjpO5lNRiKE/dSEihMBs9zETIgLGFFkqDh8n2uTBRT5qoK7MMIL8GlIs rDPA== X-Gm-Message-State: AOJu0YyGoyem5mnCBC3Xv2KJx8cUnY6GmpIX1sWz343GEIER9LvOk6nP UJKmie9v0Knf7ixfmw5QC7MvOo5HFqUf1+QqXZzsgluFkXLTbpIt4s7OUTrP8nsue/HvdhdsSNQ UOtZJMkVZaLHzCgsKiFJ1Mw== X-Received: by 2002:a05:600c:3108:b0:3fe:2a98:a24c with SMTP id g8-20020a05600c310800b003fe2a98a24cmr5793657wmo.26.1699887647274; Mon, 13 Nov 2023 07:00:47 -0800 (PST) X-Google-Smtp-Source: AGHT+IH5pJwQfMhD+Fce8KR169SV+iameBb07QjGkIatg/x0AGptKi0xRZBMHljWUcacJoCdhkgWuw== X-Received: by 2002:a05:600c:3108:b0:3fe:2a98:a24c with SMTP id g8-20020a05600c310800b003fe2a98a24cmr5793637wmo.26.1699887646828; Mon, 13 Nov 2023 07:00:46 -0800 (PST) Received: from localhost (105.226.159.143.dyn.plus.net. [143.159.226.105]) by smtp.gmail.com with ESMTPSA id fl23-20020a05600c0b9700b0040a45fffd27sm10749433wmb.10.2023.11.13.07.00.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Nov 2023 07:00:46 -0800 (PST) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH 06/24] gdb: fix bugs in {get,put}_frame_register_bytes In-Reply-To: <20231108051222.1275306-7-simon.marchi@polymtl.ca> References: <20231108051222.1275306-1-simon.marchi@polymtl.ca> <20231108051222.1275306-7-simon.marchi@polymtl.ca> Date: Mon, 13 Nov 2023 15:00:44 +0000 Message-ID: <8734x9snoj.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Simon Marchi writes: > From: Simon Marchi > > I found this only by inspection: the myaddr pointer in > {get,put}_frame_register_bytes is reset to `buffer.data ()` at each > iteration. This means that we will always use the bytes at the > beginning of `buffer` to read or write to the registers, instead of > progressing in `buffer`. > > Fix this by re-writing the functions to chip away the beginning of the > buffer array_view as we progress in reading or writing the data. > > These bugs was introduced almost 3 years ago [1], and yet nobody > complained. I'm wondering which architecture relies on that register > "overflow" behavior (reading or writing multiple consecutive registers > with one {get,put}_frame_register_bytes calls), and in which situation. > I find these functions a bit dangerous, if a caller mis-calculates > things, it could end up silently reading or writing to the next > register, even if it's not the intent. > > If I could change it, I would prefer to have functions specifically made > for that ({get,put}_frame_register_bytes_consecutive or something like > that) and make {get,put}_frame_register_bytes only able to write within > a single register (which I presume represents most of the use cases of > the current {get,put}_frame_register_bytes). If a caller mis-calculates > things and an overflow occurs while calling > {get,put}_frame_register_bytes, it would hit an assert. The problem is > knowing which callers rely on the overflow behavior and which don't. I agree that this overflow behaviour sucks. I have a memory of being told (years ago now) that this was a result of some older compilers not emitting correct DWARF for compound value locations, instead the compiler would just emit a single register location, and assume that the debugger would read from consecutive DWARF registers. Note, this code assumes that the DWARF register numbering is the same as GDB's register numbering, which is not always the case. Personally, I'd love for GDB to be more aggressive about removing some of this legacy behaviour. What I'd like to do is move things like this behind a switch, say 'set maintenance deprecated-features on|off', which would be off by default, but when it is turned on we'd print a message saying: The feature you turned this on for is deprecated, and will be removed from future versions of GDB. To avoid this feature removed, please file a bug report here describing the deprecated feature that you are using. Then if nobody complains after a couple of years, we can start deleting things. Anyway... just putting my thoughts down. I think this patch is fine. Approved-By: Andrew Burgess Thanks, Andrew > > [1] https://gitlab.com/gnutools/binutils-gdb/-/commit/bdec2917b1e94c7198ba39919f45060067952f43 > > Change-Id: I43bd4a9f7fa8419d388a2b20bdc57d652688ddf8 > --- > gdb/frame.c | 63 +++++++++++++++++++---------------------------------- > 1 file changed, 23 insertions(+), 40 deletions(-) > > diff --git a/gdb/frame.c b/gdb/frame.c > index afadb8ac4e73..b3d99163b4dc 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -1482,9 +1482,6 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, > int *optimizedp, int *unavailablep) > { > struct gdbarch *gdbarch = get_frame_arch (frame); > - int i; > - int maxsize; > - int numregs; > > /* Skip registers wholly inside of OFFSET. */ > while (offset >= register_size (gdbarch, regnum)) > @@ -1495,9 +1492,9 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, > > /* Ensure that we will not read beyond the end of the register file. > This can only ever happen if the debug information is bad. */ > - maxsize = -offset; > - numregs = gdbarch_num_cooked_regs (gdbarch); > - for (i = regnum; i < numregs; i++) > + int maxsize = -offset; > + int numregs = gdbarch_num_cooked_regs (gdbarch); > + for (int i = regnum; i < numregs; i++) > { > int thissize = register_size (gdbarch, i); > > @@ -1506,20 +1503,15 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, > maxsize += thissize; > } > > - int len = buffer.size (); > - if (len > maxsize) > + if (buffer.size () > maxsize) > error (_("Bad debug information detected: " > - "Attempt to read %d bytes from registers."), len); > + "Attempt to read %zu bytes from registers."), buffer.size ()); > > /* Copy the data. */ > - while (len > 0) > + while (!buffer.empty ()) > { > - int curr_len = register_size (gdbarch, regnum) - offset; > - > - if (curr_len > len) > - curr_len = len; > - > - gdb_byte *myaddr = buffer.data (); > + int curr_len = std::min (register_size (gdbarch, regnum) - offset, > + buffer.size ()); > > if (curr_len == register_size (gdbarch, regnum)) > { > @@ -1527,8 +1519,8 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, > CORE_ADDR addr; > int realnum; > > - frame_register (frame, regnum, optimizedp, unavailablep, > - &lval, &addr, &realnum, myaddr); > + frame_register (frame, regnum, optimizedp, unavailablep, &lval, > + &addr, &realnum, buffer.data ()); > if (*optimizedp || *unavailablep) > return false; > } > @@ -1547,13 +1539,12 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum, > return false; > } > > - memcpy (myaddr, value->contents_all ().data () + offset, > - curr_len); > + copy (value->contents_all ().slice (offset, curr_len), > + buffer.slice (0, curr_len)); > release_value (value); > } > > - myaddr += curr_len; > - len -= curr_len; > + buffer = buffer.slice (curr_len); > offset = 0; > regnum++; > } > @@ -1578,36 +1569,28 @@ put_frame_register_bytes (frame_info_ptr frame, int regnum, > regnum++; > } > > - int len = buffer.size (); > /* Copy the data. */ > - while (len > 0) > + while (!buffer.empty ()) > { > - int curr_len = register_size (gdbarch, regnum) - offset; > + int curr_len = std::min (register_size (gdbarch, regnum) - offset, > + buffer.size ()); > > - if (curr_len > len) > - curr_len = len; > - > - const gdb_byte *myaddr = buffer.data (); > if (curr_len == register_size (gdbarch, regnum)) > - { > - put_frame_register (frame, regnum, myaddr); > - } > + put_frame_register (frame, regnum, buffer.data ()); > else > { > - struct value *value > + value *value > = frame_unwind_register_value (frame_info_ptr (frame->next), > regnum); > - gdb_assert (value != NULL); > + gdb_assert (value != nullptr); > > - memcpy ((char *) value->contents_writeable ().data () + offset, > - myaddr, curr_len); > - put_frame_register (frame, regnum, > - value->contents_raw ().data ()); > + copy (buffer.slice (0, curr_len), > + value->contents_writeable ().slice (offset, curr_len)); > + put_frame_register (frame, regnum, value->contents_raw ().data ()); > release_value (value); > } > > - myaddr += curr_len; > - len -= curr_len; > + buffer = buffer.slice (curr_len); > offset = 0; > regnum++; > } > -- > 2.42.1