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.133.124]) by sourceware.org (Postfix) with ESMTPS id 0837F3858D37 for ; Fri, 13 Jan 2023 16:18:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0837F3858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673626704; 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=6nryA2ruENoF87d160idPymsSp9y12JAai+et/GlOq4=; b=B42yahtYayspi9DxNal1pQA9iFwLHfti/g2YBbiqTBsnbyIvzRejJaRKrctencorg282SB CHIxhmfZCSWocL0nHvFdgn5XC1g6/1ui/UCEVBQk63V/DYU4egLPmRgsAlEtkKNUB/P/Uf mxHE6tu4VYRItwK0ckd7ITGIujKGbkw= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-378-grDLrExgPDabqBNfXFsDPw-1; Fri, 13 Jan 2023 11:18:23 -0500 X-MC-Unique: grDLrExgPDabqBNfXFsDPw-1 Received: by mail-wr1-f71.google.com with SMTP id d7-20020adf9b87000000b002bde8f7112bso113553wrc.17 for ; Fri, 13 Jan 2023 08:18:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=6nryA2ruENoF87d160idPymsSp9y12JAai+et/GlOq4=; b=BBzOvw7IO7hxPgfKGGWyzIIzAWcdrOy58GPszAJVj9MS/Q3YI9WlqjehGwty+DubUL jjR/uWclqbC3OnY0N/Tv7HiTEzDQz2wt4ajORi7VVo/FyxRl0umrKa7S2dBsFa7LOtZl Ps0KFEDtgQe46X4IypTCGpotxSHucQjUpT/UlT+JA0zI8jaqrKhcrP6T5WE/Ikz/0cRR vyWahb5H9sw5qEk3nD8zy6yLvmCFjy60waQ3zDgwOfFHdQRVSDAGcTfxGsKkRoYP+j7U b4U2SNgjw2zCJZ4O4JSnyRwcANGnNIkWXxh0GmGDNpCklHxdjIGnAyl5ZwUKLWqiVm85 8KAw== X-Gm-Message-State: AFqh2koB14PnTM5lMBPbleFNOp+HWkk4PCrGMQ26kuC3pBZw9nzpdxJt bMC71gB9jh93cge6aECmt7EIyZvJ2KNukb09Wr90w1KinD31dNKE28CheDH/SAy4iaZUQbs2TCT /1GPUVm9edgVOJwOXv1nuKw== X-Received: by 2002:a05:600c:43d6:b0:3d2:3831:e5c4 with SMTP id f22-20020a05600c43d600b003d23831e5c4mr168639wmn.40.1673626702396; Fri, 13 Jan 2023 08:18:22 -0800 (PST) X-Google-Smtp-Source: AMrXdXuwGcR+6VaKuUtA0ISY5W5wtxlqcv3Zxz/QVhYdY6iFsuSSLW3ADzLqILr2wuT6x4HZNqgnQQ== X-Received: by 2002:a05:600c:43d6:b0:3d2:3831:e5c4 with SMTP id f22-20020a05600c43d600b003d23831e5c4mr168624wmn.40.1673626702122; Fri, 13 Jan 2023 08:18:22 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id n14-20020a05600c3b8e00b003b49bd61b19sm34067780wms.15.2023.01.13.08.18.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 Jan 2023 08:18:21 -0800 (PST) From: Andrew Burgess To: "Maciej W. Rozycki" , gdb-patches@sourceware.org Cc: Tom Tromey , Richard Bunt Subject: Re: [PATCH v2 1/5] GDB: Ignore `max-value-size' setting with value history accesses In-Reply-To: References: Date: Fri, 13 Jan 2023 16:18:20 +0000 Message-ID: <87zgamflwz.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=-5.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "Maciej W. Rozycki" writes: > We have an inconsistency in value history accesses where array element > accesses cause an error for entries exceeding the currently selected > `max-value-size' setting even where such accesses successfully complete > for elements located in the inferior, e.g.: > > (gdb) p/d one > $1 = 0 > (gdb) p/d one_hundred > $2 = {0 } > (gdb) p/d one_hundred[99] > $3 = 0 > (gdb) set max-value-size 25 > (gdb) p/d one_hundred > value requires 100 bytes, which is more than max-value-size > (gdb) p/d one_hundred[99] > $7 = 0 > (gdb) p/d $2 > value requires 100 bytes, which is more than max-value-size > (gdb) p/d $2[99] > value requires 100 bytes, which is more than max-value-size > (gdb) > > According to our documentation the `max-value-size' setting is a safety > guard against allocating an overly large amount of memory. Moreover a > statement in documentation says, concerning this setting, that: "Setting > this variable does not effect values that have already been allocated > within GDB, only future allocations." While in the implementer-speak > the sentence may be unambiguous I think the outside user may well infer > that the setting only applies to values that need to be retrieved from > the debuggee. > > Therefore rather than just fixing this inconsistency it seems reasonable > to lift the setting for value history accesses, under an implication > that by having been retrieved from the debuggee they have already passed > the safety check. Do it then, making the last two commands succeed: > > (gdb) p/d $2 > $8 = {0 } > (gdb) p/d $2 [99] > $9 = 0 > (gdb) > > Expand the testsuite accordingly. > --- > gdb/testsuite/gdb.base/max-value-size.exp | 3 + > gdb/value.c | 56 ++++++++++++++++++++---------- > 2 files changed, 42 insertions(+), 17 deletions(-) > > gdb-value-history-size.diff > Index: src/gdb/testsuite/gdb.base/max-value-size.exp > =================================================================== > --- src.orig/gdb/testsuite/gdb.base/max-value-size.exp > +++ src/gdb/testsuite/gdb.base/max-value-size.exp > @@ -53,6 +53,9 @@ proc do_value_printing { max_value_size > gdb_test "p/d one_hundred" " = \\{0 \\}" > } > gdb_test "p/d one_hundred \[99\]" " = 0" > + # Verify that accessing value history is undisturbed. > + gdb_test "p/d \$2" " = \\{0 \\}" > + gdb_test "p/d \$2 \[99\]" " = 0" > } > } > > Index: src/gdb/value.c > =================================================================== > --- src.orig/gdb/value.c > +++ src/gdb/value.c > @@ -1034,31 +1034,42 @@ check_type_length_before_alloc (const st > } > } > > -/* Allocate the contents of VAL if it has not been allocated yet. */ > +/* Allocate the contents of VAL if it has not been allocated yet. > + If CHECK_SIZE is true, then apply the usual max-value-size checks. */ > > static void > -allocate_value_contents (struct value *val) > +allocate_value_contents (struct value *val, bool check_size) > { > if (!val->contents) > { > - check_type_length_before_alloc (val->enclosing_type); > + if (check_size) > + check_type_length_before_alloc (val->enclosing_type); > val->contents.reset > ((gdb_byte *) xzalloc (val->enclosing_type->length ())); > } > } > > -/* Allocate a value and its contents for type TYPE. */ > +/* Allocate a value and its contents for type TYPE. If CHECK_SIZE is true, > + then apply the usual max-value-size checks. */ > > -struct value * > -allocate_value (struct type *type) > +static struct value * > +allocate_value (struct type *type, bool check_size) > { > struct value *val = allocate_value_lazy (type); > > - allocate_value_contents (val); > + allocate_value_contents (val, check_size); > val->lazy = 0; > return val; > } > > +/* Allocate a value and its contents for type TYPE. */ > + > +struct value * > +allocate_value (struct type *type) > +{ > + return allocate_value (type, true); > +} > + > /* Allocate a value that has the correct length > for COUNT repetitions of type TYPE. */ > > @@ -1169,7 +1180,7 @@ value_contents_raw (struct value *value) > struct gdbarch *arch = get_value_arch (value); > int unit_size = gdbarch_addressable_memory_unit_size (arch); > > - allocate_value_contents (value); > + allocate_value_contents (value, true); > > ULONGEST length = value_type (value)->length (); > return gdb::make_array_view > @@ -1179,7 +1190,7 @@ value_contents_raw (struct value *value) > gdb::array_view > value_contents_all_raw (struct value *value) > { > - allocate_value_contents (value); > + allocate_value_contents (value, true); > > ULONGEST length = value_enclosing_type (value)->length (); > return gdb::make_array_view (value->contents.get (), length); > @@ -1752,12 +1763,14 @@ value_release_to_mark (const struct valu > return result; > } > > -/* Return a copy of the value ARG. > - It contains the same contents, for same memory address, > - but it's a different block of storage. */ > +/* Return a copy of the value ARG. It contains the same contents, > + for the same memory address, but it's a different block of storage. > + If CHECK_SIZE is true, then throw an exception whenever the size > + of memory allocated for the contents of the value would exceed > + max-value-size. */ > > -struct value * > -value_copy (const value *arg) > +static struct value * > +value_copy (const value *arg, bool check_size) > { > struct type *encl_type = value_enclosing_type (arg); > struct value *val; > @@ -1765,7 +1778,7 @@ value_copy (const value *arg) > if (value_lazy (arg)) > val = allocate_value_lazy (encl_type); > else > - val = allocate_value (encl_type); > + val = allocate_value (encl_type, check_size); I wonder, maybe value_copy should never check the max-value-size. As you point out above, the max-value-size was introduced to catch cases where attempting to read a value from the inferior would cause GDB to try an allocate a stupid amount of memory. We don't currently have any mechanism in GDB to try an cap the cumulative memory usage across all values, which suggests that currently, once a value has been read into GDB we assume we can safely make as many copies as we want. And so, is there any reason why value_copy shouldn't always disable the size check? Thanks, Andrew > val->type = arg->type; > VALUE_LVAL (val) = arg->lval; > val->location = arg->location; > @@ -1802,6 +1815,15 @@ value_copy (const value *arg) > return val; > } > > +/* Return a copy of the value ARG. It contains the same contents, > + for the same memory address, but it's a different block of storage. */ > + > +struct value * > +value_copy (const value *arg) > +{ > + return value_copy (arg, true); > +} > + > /* Return a "const" and/or "volatile" qualified version of the value V. > If CNST is true, then the returned value will be qualified with > "const". > @@ -1965,7 +1987,7 @@ access_value_history (int num) > > absnum--; > > - return value_copy (value_history[absnum].get ()); > + return value_copy (value_history[absnum].get (), false); > } > > /* See value.h. */ > @@ -4162,7 +4184,7 @@ void > value_fetch_lazy (struct value *val) > { > gdb_assert (value_lazy (val)); > - allocate_value_contents (val); > + allocate_value_contents (val, true); > /* A value is either lazy, or fully fetched. The > availability/validity is only established as we try to fetch a > value. */