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 16BCD385843D for ; Tue, 28 Feb 2023 16:53:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 16BCD385843D 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=1677603206; 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=//rCrhZmK40UFKML3AHSpJT5UsXowt/D+WGYAc7/aaY=; b=VRaBWsR+UomQAD32SMYSxPL3BfTnKuFLL6suNUbGRYOEcuA4AVu8ICNN5X2ZRktXN5m8iG nmKbe5mL34o+8dWGkmrJYzdbLymiSzLhWgyZ4/p70PJEU06a/FsXPI/zse4hdj3YprYy+v GKIldoFAZIEEejcjjs0TvlflmrvR+oU= Received: from mail-ed1-f69.google.com (mail-ed1-f69.google.com [209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-211-OeWqY-5kMR2-yX0TBfqWFQ-1; Tue, 28 Feb 2023 11:53:25 -0500 X-MC-Unique: OeWqY-5kMR2-yX0TBfqWFQ-1 Received: by mail-ed1-f69.google.com with SMTP id ck7-20020a0564021c0700b004a25d8d7593so14826220edb.0 for ; Tue, 28 Feb 2023 08:53:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677603203; 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=//rCrhZmK40UFKML3AHSpJT5UsXowt/D+WGYAc7/aaY=; b=eycWATMqOUadVHx7O6DIqt5iv64vmdoQ7k1Xij2Sr3iTEEGqDzcwEYcwVIDlDO6XuS tZ+zJUizBkz+ip5O1gQfr04siJypZT8Q+JEjvFf/W9RmcHhSRWQNisyuv4Nx6ZQCgeOI lZiQRWsbqtso/3PDfTEEwtkWj/f9LYVrsU30cyHpwerw9U4LE+GSNf1Hattjyp3k5a+4 28B5TEjiZQDr/QpmuCn+on2xnbovE+9MyY7R+MPTbqkAXGZ9AWgT61WL/YNqUbHb2Bs0 0hA+Aooc9+pxMBAFMlWFC6bIWSuFzCqsE42AJ07F/79ystC4YKOuy/RuZS2Arvv2hInL oOKA== X-Gm-Message-State: AO0yUKUOgiiWUwvqTUDexxrMoIGRopSRaKu+u6zRV7m5jbEP51r4BGUc LUNRbs99gQGJ/Y1BbZJzILo0Pc4rdQ9XhE1Y2mb/rJcC/L2cagOgKXXYYcfSpwQTvdEAxEJjrcf XnDiMdNuTnQGmQI/7bke5/KEyvcQxga2Hra40bPSaJZ3nialhlEuTSWn10fxzkL9zQXCDqr78fy C8Jco= X-Received: by 2002:aa7:dd43:0:b0:4ac:b69a:2f06 with SMTP id o3-20020aa7dd43000000b004acb69a2f06mr4098113edw.0.1677603203584; Tue, 28 Feb 2023 08:53:23 -0800 (PST) X-Google-Smtp-Source: AK7set9dDOGakyidHkhIADZKINEixgb7beZqd28CGzd8wBqPTkbaeGOsL5wj28xcyQPczUloYfW0BQ== X-Received: by 2002:aa7:dd43:0:b0:4ac:b69a:2f06 with SMTP id o3-20020aa7dd43000000b004acb69a2f06mr4098088edw.0.1677603203095; Tue, 28 Feb 2023 08:53:23 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id a16-20020a170906469000b008de729c8a03sm4729285ejr.38.2023.02.28.08.53.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 Feb 2023 08:53:22 -0800 (PST) From: Andrew Burgess To: Simon Marchi via Gdb-patches , gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook In-Reply-To: <87fsapj13v.fsf@redhat.com> References: <20230227212806.68474-1-simon.marchi@efficios.com> <87fsapj13v.fsf@redhat.com> Date: Tue, 28 Feb 2023 16:53:21 +0000 Message-ID: <878rghivem.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_H2,SPF_HELO_NONE,SPF_NONE,TXREP 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: Andrew Burgess writes: > Simon Marchi via Gdb-patches writes: > >> With the current GDB master, the amdgcn architectures fail to >> initialize: >> >> $ ./gdb -nx -q --data-directory=data-directory -ex "set arch amdgcn:gfx1010" >> /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:517: internal-error: verify_gdbarch: the following are invalid ... >> return_value_as_value >> >> This must have been because of a race condition between merging the >> AMDGPU support and commit 4e1d2f5814b ("Add new overload of >> gdbarch_return_value") (i.e. I probably didn't re-test when rebasing >> over the latter). >> >> The new gdbarch_return_value_as_value hook as a check that verifies that > > typo: "...hook HAS a check..." > >> exactly one of return_value_as_value (the new one) and return_value (the >> deprecated one) is set. The intent is to catch cases where an >> architecture would set both, which we don't want. However, it fails to >> consider architecture defining neither, like the amdgcn architecture in >> today's master branch. >> >> The downstream port already has gdbarch_return_value implemented, and we >> will send that upstream shortly (and we should probably migrate to >> gdbarch_return_value_as_value before doing that), so the problem will >> disappear then. However, I think it would be nice to fix the situation >> to allow ports not to define return_value nor return_value_as_value. I >> think this can be useful when writing new ports, to avoid having to >> define a dummy return_value_as_value just for the gdbarch validation to >> pass. > > Looking at all the places you've added error checks too, I think I > disagree with you. > > It feels like this functionality (the gdbarch_return_value callbacks) is > pretty critical to GDB, so any serious port is going to have to > implement this sooner or later. > > I think it's perfectly reasonable to expect new port authors to add a > stubbed out function while getting their new port off the ground. And > personally, I'd rather require new port authors to do that than have GDB > carry around a bunch of error checks that only exist for some code that > is mostly out of tree. > > [ NOTE: I know the ROCm code is in tree without the gdbarch_return_value > check, but I think that is because there was no error check for this > field when the ROCm code was first posted. If there had been, you'd > have included a stub function. ] > >> >> The current behavior is to install a default return_value_as_value >> callback (default_gdbarch_return_value) which offloads to >> gdbarch_return_value. Architectures that have been updated to use the >> new callback override it to set their own return_value_as_value >> callback. The "invalid" predicate for return_value_as_value will then >> flag the cases where an architecture sets both or sets neither. >> >> With this patch, the goal is to have a gdbarch_return_value_as_value_p >> that returns true if either return_value_as_value or return_value is >> set. Make both callbacks start as nullptr, and make >> return_value_as_value have a postdefault that installs >> default_gdbarch_return_value if it hasn't been set but return_value has >> been. To summarize all cases: >> >> - If the arch sets only return_value_as_value, it stays as is and >> gdbarch_return_value_as_value_p returns true >> - If the arch sets only return_value, we install >> default_gdbarch_return_value in return_value_as_value (which offloads >> to return_value) and gdbarch_return_value_as_value_p returns true >> - If the arch sets neither, both fields stay nullptr and >> gdbarch_return_value_as_value_p returns false >> - If the arch sets both, we unfortunately don't flag it as an error, as >> we do today. The current implementation of gdbarch.py doesn't let us >> have an "invalid" check at the same time as a predicate function, for >> some reason. But gdbarch_return_value_as_value_p will return true >> in that case. > > As I understand it the gdbarch.py algorithm was just copied over from > the old shell scripts. The old shell script algorithm was, I suspect > rather hacked together. > > The comments around this code hint that and "invalid" check doesn't make > sense when we have a predicate because the predicate would just return > false for an invalid value, so why would you want an earlier validity > check. > > I think I disagree with this reasoning, and think it makes perfect sense > to offer both a validity check and a predicate for the same component. > I'd be happy to see us move in this direction. > >> >> With that in place, add some checks before all uses of >> gdbarch_return_value_as_value. On failure, call >> error_arch_no_return_value, which throws with a standard error >> message. > > This is another area where I think the gdbarch.py generation code could > be improved. It seems a shame that we need to remember to place a > predicate check before every call to gdbarch_return_value_as_value, > surely it would be better if we could inject this error check directly > into the generated gdbarch_return_value_as_value code? We already have > 'param_checks' and 'result_checks' which are really for adding asserts, > but maybe we could/should add some new field that would trigger an error > call. > >> >> I don't see a good way of testing this. I manually tested it using the >> downstream ROCm-GDB port, removed the return_value hook from the amdgcn >> arch, and tried to "finish" a function: >> >> (gdb) finish >> Architecture amdgcn:gfx900 does not implement extracting return values from the inferior. >> >> This hits the gdbarch_return_value_as_value_p call in finish_command >> (which triggers when trying to figure out the return value convention, >> before resuming the inferior). I don't know how if the other errors >> paths I added leave GDB in a sane state though, they are a bit more >> difficult to test. > > If almost feels like we should push these error checks to the beginning > of the command, e.g. when the user tries to 'finish' we error before > doing anything because we know we can't extract the return value. > > Or maybe we shouldn't be throwing an error in some of these cases, maybe > in some cases we could warn and continue, just without the return value > fetching? > > As I said above, I don't actually like GDB trying to handling this case > at all, I'd much rather we just force the port to stub these functions > during bring up. > >> >> Change-Id: I8e0728bf9c42520dac7952bfa3f6f50c5737fa7b >> --- >> gdb/arch-utils.c | 11 +++++++++++ >> gdb/arch-utils.h | 6 ++++++ >> gdb/elfread.c | 4 ++++ >> gdb/gdbarch-gen.h | 2 ++ >> gdb/gdbarch.c | 15 ++++++++++++--- >> gdb/gdbarch_components.py | 4 ++-- >> gdb/infcall.c | 4 ++++ >> gdb/infcmd.c | 6 ++++++ >> gdb/stack.c | 4 ++++ >> gdb/value.c | 3 +++ >> 10 files changed, 54 insertions(+), 5 deletions(-) >> >> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c >> index e3af9ce2dbce..1282b4f8a773 100644 >> --- a/gdb/arch-utils.c >> +++ b/gdb/arch-utils.c >> @@ -1478,6 +1478,17 @@ target_gdbarch (void) >> return current_inferior ()->gdbarch; >> } >> >> +/* See arch-utils.h. */ >> + >> +void >> +error_arch_no_return_value (gdbarch *arch) >> +{ >> + const bfd_arch_info *info = gdbarch_bfd_arch_info (arch); >> + >> + error (_("Architecture %s does not implement extracting return values from " >> + "the inferior."), info->printable_name); >> +} >> + >> void _initialize_gdbarch_utils (); >> void >> _initialize_gdbarch_utils () >> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h >> index 56690f0fd435..d00753ec32b4 100644 >> --- a/gdb/arch-utils.h >> +++ b/gdb/arch-utils.h >> @@ -314,4 +314,10 @@ extern enum return_value_convention default_gdbarch_return_value >> struct regcache *regcache, struct value **read_value, >> const gdb_byte *writebuf); >> >> +/* Error out with a message saying that ARCH does not support extracting return >> + values from the inferior. */ >> + >> +extern void error_arch_no_return_value (gdbarch *arch) >> + ATTRIBUTE_NORETURN; >> + >> #endif /* ARCH_UTILS_H */ >> diff --git a/gdb/elfread.c b/gdb/elfread.c >> index ca684aab57ea..24df62b1fccb 100644 >> --- a/gdb/elfread.c >> +++ b/gdb/elfread.c >> @@ -1038,6 +1038,10 @@ elf_gnu_ifunc_resolver_return_stop (code_breakpoint *b) >> func_func->set_address (b->loc->related_address); >> >> value = value::allocate (value_type); >> + >> + if (!gdbarch_return_value_as_value_p (gdbarch)) >> + error_arch_no_return_value (gdbarch); >> + >> gdbarch_return_value_as_value (gdbarch, func_func, value_type, regcache, >> &value, NULL); >> resolved_address = value_as_address (value); >> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h >> index ddb97f60315f..00e6960f9cef 100644 >> --- a/gdb/gdbarch-gen.h >> +++ b/gdb/gdbarch-gen.h >> @@ -453,6 +453,8 @@ extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_va >> to force the value returned by a function (see the "return" command >> for instance). */ >> >> +extern bool gdbarch_return_value_as_value_p (struct gdbarch *gdbarch); >> + >> typedef enum return_value_convention (gdbarch_return_value_as_value_ftype) (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf); >> extern enum return_value_convention gdbarch_return_value_as_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf); >> extern void set_gdbarch_return_value_as_value (struct gdbarch *gdbarch, gdbarch_return_value_as_value_ftype *return_value_as_value); >> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c >> index 7e4e34d5aca0..42c4f16a67c8 100644 >> --- a/gdb/gdbarch.c >> +++ b/gdb/gdbarch.c >> @@ -112,7 +112,7 @@ struct gdbarch >> gdbarch_address_to_pointer_ftype *address_to_pointer = unsigned_address_to_pointer; >> gdbarch_integer_to_address_ftype *integer_to_address = nullptr; >> gdbarch_return_value_ftype *return_value = nullptr; >> - gdbarch_return_value_as_value_ftype *return_value_as_value = default_gdbarch_return_value; >> + gdbarch_return_value_as_value_ftype *return_value_as_value = nullptr; >> gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = default_get_return_buf_addr; >> gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = default_return_in_first_hidden_param_p; >> gdbarch_skip_prologue_ftype *skip_prologue = 0; >> @@ -367,8 +367,7 @@ verify_gdbarch (struct gdbarch *gdbarch) >> /* Skip verify of address_to_pointer, invalid_p == 0 */ >> /* Skip verify of integer_to_address, has predicate. */ >> /* Skip verify of return_value, invalid_p == 0 */ >> - if ((gdbarch->return_value_as_value == default_gdbarch_return_value) == (gdbarch->return_value == nullptr)) >> - log.puts ("\n\treturn_value_as_value"); >> + /* Skip verify of return_value_as_value, has predicate. */ >> /* Skip verify of get_return_buf_addr, invalid_p == 0 */ >> /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */ >> if (gdbarch->skip_prologue == 0) >> @@ -778,6 +777,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file) >> gdb_printf (file, >> "gdbarch_dump: return_value = <%s>\n", >> host_address_to_string (gdbarch->return_value)); >> + gdb_printf (file, >> + "gdbarch_dump: gdbarch_return_value_as_value_p() = %d\n", >> + gdbarch_return_value_as_value_p (gdbarch)); >> gdb_printf (file, >> "gdbarch_dump: return_value_as_value = <%s>\n", >> host_address_to_string (gdbarch->return_value_as_value)); >> @@ -2574,6 +2576,13 @@ set_gdbarch_return_value (struct gdbarch *gdbarch, >> gdbarch->return_value = return_value; >> } >> >> +bool >> +gdbarch_return_value_as_value_p (struct gdbarch *gdbarch) >> +{ >> + gdb_assert (gdbarch != NULL); >> + return gdbarch->return_value_as_value != NULL; >> +} >> + >> enum return_value_convention >> gdbarch_return_value_as_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf) >> { >> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py >> index caa65c334eca..7ceecbf5d223 100644 >> --- a/gdb/gdbarch_components.py >> +++ b/gdb/gdbarch_components.py >> @@ -902,11 +902,11 @@ for instance). >> ("struct value **", "read_value"), >> ("const gdb_byte *", "writebuf"), >> ], >> - predefault="default_gdbarch_return_value", >> # If we're using the default, then the other method must be set; >> # but if we aren't using the default here then the other method >> # must not be set. > > I don't think this comment aligns with the postdefault code. It says > "If we're using the default, ..." but "we" here is > 'return_value_as_value', but we're actually checking 'return_value'. > >> - invalid="(gdbarch->return_value_as_value == default_gdbarch_return_value) == (gdbarch->return_value == nullptr)", >> + postdefault="gdbarch->return_value != nullptr ? default_gdbarch_return_value : nullptr", > > If you search for the postdefault string you'll notice this code is not > actually generated anywhere! This is a consequence of also having > defined a predicate. > > As I say above, the gdbarch.py algorithm could do with some updating in > a few cases. > > The good news is, I already have a patch that fixes this problem. I was > going to include a link to it here, but turns out I never actually > posted it! I'm going to try to get that post on the list today, I've > tried it locally, and with my patch your postdefault code is generated > correctly. Got the patch posted, its this one: https://sourceware.org/pipermail/gdb-patches/2023-February/197537.html Thanks, Andrew