From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id wh1uBl/eq2RWnBwAWB0awg (envelope-from ) for ; Mon, 10 Jul 2023 06:33:03 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=I8a9qbmM; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id CB1521E0BD; Mon, 10 Jul 2023 06:33:02 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id B40C71E00F for ; Mon, 10 Jul 2023 06:33:00 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 029E2385772B for ; Mon, 10 Jul 2023 10:33:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 029E2385772B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1688985180; bh=UdDuQHRtKrsNwSitUIAa2gyv734xv00QXJDwRHOsycU=; h=To:Subject:In-Reply-To:References:Date:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=I8a9qbmMNGpVVSi43PcMApfjYsfVYVAnOd+u6i36DOEeGEux6BcpLT/xJsNhlPkmS Oa5mefa9CBvX/bMvQ/XYAOgfwrbdEf6WrT+Zz30jH1Ouo6tDDq7qTa3MJzVQYoO7Ap kvc7HEttEasLcJaH5ekfJ8dlMFGbHPXR4/W4ql+c= 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 436BD3857704 for ; Mon, 10 Jul 2023 10:32:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 436BD3857704 Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-155-EtIPKKSfMbKB2pAu5Vb4eg-1; Mon, 10 Jul 2023 06:32:37 -0400 X-MC-Unique: EtIPKKSfMbKB2pAu5Vb4eg-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-3f5df65f9f4so26551165e9.2 for ; Mon, 10 Jul 2023 03:32:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688985156; x=1691577156; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=UdDuQHRtKrsNwSitUIAa2gyv734xv00QXJDwRHOsycU=; b=TmAzTihKdr4lYrCNG50/QDkE+iXyCj5JioXp6TEn7qUcamSI7uHzyjHZn6P8UXdRux zIkHtLDE/R+mPoUATLrPHru7p1k+8JYAoLtu/IzrrpKs8tPY1IhoGTo3kdsNMmziUpNf ykIZs3YGhp68D6ptB8qnOax4lpN6+5s+ujSpU6ghDj17Rwd68f76pu/SbTRz/fcQSkoe 9uLoJRCKFrnhkqYhyszNv1Ke0UKtOtxOZfM6ArmG0hAlDFyS0fYFJ2ggo00vtISWeUT0 VzYPKwBt0hcwp2PuLX7KLUxRPyZAGWT3j1+ybUqSltI9uBDJKH8QgWcrx9Tl/laWlCD3 GmHA== X-Gm-Message-State: ABy/qLYZTM2UpHXeyhjkxzECBWvN5BeY/Oh0aOBW4iC8WmaWDN2vxS4D zPO6cbmp5kcGXWQkmo2exzv5A94Vo/mlKXoTM3Di9K7WzslQpIljMRPLZgV+I9cmZ6XQCrd+Owi SThZtUQoZgKJlokaN+x5lfQ== X-Received: by 2002:a05:600c:c8:b0:3fb:b05d:f27c with SMTP id u8-20020a05600c00c800b003fbb05df27cmr10229694wmm.34.1688985156485; Mon, 10 Jul 2023 03:32:36 -0700 (PDT) X-Google-Smtp-Source: APBJJlErNpn53tLeuno5qH9JkxoRP+9xANEw46qYSMRrtEUZ88h2j3K/Om90NxYM8+QzgieuKAt5Tw== X-Received: by 2002:a05:600c:c8:b0:3fb:b05d:f27c with SMTP id u8-20020a05600c00c800b003fbb05df27cmr10229672wmm.34.1688985155962; Mon, 10 Jul 2023 03:32:35 -0700 (PDT) Received: from localhost (2.72.115.87.dyn.plus.net. [87.115.72.2]) by smtp.gmail.com with ESMTPSA id y23-20020a1c4b17000000b003fbd0c50ba2sm9897980wma.32.2023.07.10.03.32.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Jul 2023 03:32:35 -0700 (PDT) To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCHv5 03/11] gdbserver: allow agent expressions to fail with invalid memory access In-Reply-To: <2d33fb4e-380a-310c-d4b4-cf3dfe69eaec@palves.net> References: <0ef54ed920e1262a07f4a061ecc08bea8fcaee23.1678987897.git.aburgess@redhat.com> <01059f8a-0e59-55b5-f530-190c26df5ba3@palves.net> <87pm53k7om.fsf@redhat.com> <2d33fb4e-380a-310c-d4b4-cf3dfe69eaec@palves.net> Date: Mon, 10 Jul 2023 11:32:34 +0100 Message-ID: <87edlgjbul.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.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Andrew Burgess via Gdb-patches Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Pedro Alves writes: > On 2023-07-07 17:28, Andrew Burgess wrote: >> Pedro Alves writes: >> >>> On 2023-03-16 17:36, Andrew Burgess via Gdb-patches wrote: >>>> This commit extends gdbserver to take account of a failed memory >>>> access from agent_mem_read, and to return a new eval_result_type >>>> expr_eval_invalid_memory_access. >>>> >>>> I have only updated the agent_mem_read calls related directly to >>>> reading memory, I have not updated any of the calls related to >>>> tracepoint data collection. This is just because I'm not familiar >>>> with that area of gdb/gdbserver, and I don't want to break anything, >>>> so leaving the existing behaviour untouched seems like the safest >>>> approach. >>> >>> I think this should update the gdbserver/tracepoint.cc:eval_result_names array at least, >>> though. Otherwise I think a memory error during collection is going to trigger undefined >>> behavior, crash if we're lucky. >> >> Good spot. >> >> What are your thoughts on the patch below that should address this >> issue? > > Hmm. If we're changing how the array is built completely, you could go a step > further and convert to a .def file (like e.g., gdb/unwind_stop_reasons.def) that is > included by both ax.h and tracepoint.cc. That would keep the enum names and their > string representations in the same place. It would also dispense with the > diagnostics pragmas, as the problem scenario goes away by design. Thanks for the suggestion. Here's an attempt to switch to a .def file. Thoughts? Thanks, Andrew --- commit 75a86840a474097a19f4d04c9232981df3bf8066 Author: Andrew Burgess Date: Fri Jul 7 17:18:46 2023 +0100 gdbserver: handle all eval_result_type values in tracepoint.cc It was pointed out[1] that after this commit: commit 3812b38d8de5804ad3eadd6c7a5d532402ddabab Date: Thu Oct 20 11:14:33 2022 +0100 gdbserver: allow agent expressions to fail with invalid memory access Now that agent expressions might fail with the error expr_eval_invalid_memory_access, we might overflow the eval_result_names array in tracepoint.cc. This is because the eval_result_names array does not include a string for either expr_eval_invalid_goto or expr_eval_invalid_memory_access. I don't know if having expr_eval_invalid_goto missing is also a problem, but it feels like eval_result_names should just include a string for every possible error. I could just add two more strings into the array, but I figure that a more robust solution will be to move all of the error types, and their associated strings, into a new ax-result-types.def file, and to then include this file in both ax.h and tracepoint.cc in order to build the enum eval_result_type and the eval_result_names string array. Doing this means it is impossible to have a missing error string in the future. [1] https://inbox.sourceware.org/gdb-patches/01059f8a-0e59-55b5-f530-190c26df5ba3@palves.net/ diff --git a/gdbserver/ax-result-types.def b/gdbserver/ax-result-types.def new file mode 100644 index 00000000000..3a832c44be2 --- /dev/null +++ b/gdbserver/ax-result-types.def @@ -0,0 +1,44 @@ +/* Agent expression result types. + + Copyright (C) 2023 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +/* The AX_RESULT_TYPE macro is used to define a result type that can be + generated by agent expression evaluation. The first macro argument is + tye name of an enum entry, and the second is a string that describes + this result type. */ + +AX_RESULT_TYPE (expr_eval_no_error, + "terror:no error") +AX_RESULT_TYPE (expr_eval_empty_expression, + "terror:empty expression") +AX_RESULT_TYPE (expr_eval_empty_stack, + "terror:empty stack") +AX_RESULT_TYPE (expr_eval_stack_overflow, + "terror:stack overflow") +AX_RESULT_TYPE (expr_eval_stack_underflow, + "terror:stack underflow") +AX_RESULT_TYPE (expr_eval_unhandled_opcode, + "terror:unhandled opcode") +AX_RESULT_TYPE (expr_eval_unrecognized_opcode, + "terror:unrecognized opcode") +AX_RESULT_TYPE (expr_eval_divide_by_zero, + "terror:divide by zero") +AX_RESULT_TYPE (expr_eval_invalid_goto, + "terror:invalid goto") +AX_RESULT_TYPE (expr_eval_invalid_memory_access, + "terror:invalid memory access") diff --git a/gdbserver/ax.h b/gdbserver/ax.h index c98e36a83c6..60e307ca42d 100644 --- a/gdbserver/ax.h +++ b/gdbserver/ax.h @@ -33,16 +33,9 @@ struct traceframe; enum eval_result_type { - expr_eval_no_error, - expr_eval_empty_expression, - expr_eval_empty_stack, - expr_eval_stack_overflow, - expr_eval_stack_underflow, - expr_eval_unhandled_opcode, - expr_eval_unrecognized_opcode, - expr_eval_divide_by_zero, - expr_eval_invalid_goto, - expr_eval_invalid_memory_access +#define AX_RESULT_TYPE(ENUM,STR) ENUM, +#include "ax-result-types.def" +#undef AX_RESULT_TYPE }; struct agent_expr diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc index 1f31e0cc05f..609d49a87ef 100644 --- a/gdbserver/tracepoint.cc +++ b/gdbserver/tracepoint.cc @@ -859,14 +859,9 @@ static struct tracepoint *last_tracepoint; static const char * const eval_result_names[] = { - "terror:in the attic", /* this should never be reported */ - "terror:empty expression", - "terror:empty stack", - "terror:stack overflow", - "terror:stack underflow", - "terror:unhandled opcode", - "terror:unrecognized opcode", - "terror:divide by zero" +#define AX_RESULT_TYPE(ENUM,STR) STR, +#include "ax-result-types.def" +#undef AX_RESULT_TYPE }; #endif