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 D115C3857B9B for ; Thu, 27 Oct 2022 10:38:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D115C3857B9B 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=1666867134; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=f/z0bcVZxB9h/KBXkT/LbfRNyZsT0e3cLcktqbRbajc=; b=eSaJyYhelDI7tQmB2hhs+MMNrQrI5GvX/9oS7hiDvaqpnbY7NAqK1Ihc/zvAoFt7qPSuxY lSZFOKAMZkMaceGxBGm15CkMgiN8mGse/rzlq+f2bp1PXFWSj5lfL5XhcdpwZmT4B+CGxa m1HMR0FSzBqyAWUSyCiQ8FRjwrvhg1g= 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_128_GCM_SHA256) id us-mta-563-gRYf6ocNP9OXlG3j_NfZSw-1; Thu, 27 Oct 2022 06:38:47 -0400 X-MC-Unique: gRYf6ocNP9OXlG3j_NfZSw-1 Received: by mail-wm1-f70.google.com with SMTP id az11-20020a05600c600b00b003c6e3d4d5b1so516476wmb.7 for ; Thu, 27 Oct 2022 03:38:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding: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=9mCBAioL50Z+J7M5/gwVLbsI7XIFwNtGYjdGu2xfkq0=; b=F/30sGbJexHg6XtSOraHVsGbi4O1Myf3z8E7YHJcjmNpucoNoaddWeJRKLpouxsmAX tnt4fl6nABxNcjH5qr3ulAXoQnoQRtZPf1xI1KytP3FkItmFEN6zQUNgHfERdVHNynF8 El16VzKh5x1w2mOUI2owfiyBV0f9xcOoPkOE1iUj/I8VShjpzBBym/+mdIu6882zKGis UxWaqg4NpMT3csRr7AePiASHdO9JvcNS9VwMRiLQ4a8oT2HKoQq0UT6ArcWatI9bgCsG zWrvg3z8Mo8VehazBwbZV+M68fY1dyG3itNd98ULZZgTE3typQ96z+mJBDbxo5YBTFaW ha0w== X-Gm-Message-State: ACrzQf2kqvnj5Tt9Yhl5ml8YUsNKtvBygcfK+bpvVBiioTAf3axzhDwS EzoRoU7Gu6grHKkkrGp1M+WyUHtNhKdPuBm5w2Bnpinyor5m7u6YwaruwdFm/ChP6sZT0Pc4oQj gns5DIa6eY5dvoXpETs1fvQ== X-Received: by 2002:a05:600c:4449:b0:3c6:fb65:2463 with SMTP id v9-20020a05600c444900b003c6fb652463mr5409504wmn.128.1666867126401; Thu, 27 Oct 2022 03:38:46 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7XQdWhB1TqUympOiE20ZWMmwGmWtCCcOts73odWPLf6YAQkx/rQZPflpswyviuhipN/Evk1w== X-Received: by 2002:a05:600c:4449:b0:3c6:fb65:2463 with SMTP id v9-20020a05600c444900b003c6fb652463mr5409492wmn.128.1666867126085; Thu, 27 Oct 2022 03:38:46 -0700 (PDT) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id c12-20020a5d63cc000000b00228daaa84aesm832129wrw.25.2022.10.27.03.38.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Oct 2022 03:38:45 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/python: avoid throwing an exception over libopcodes code In-Reply-To: <3ac49ef4-4bcf-2abc-4e06-0c9f3b8c9991@simark.ca> References: <20221024125016.2823862-1-aburgess@redhat.com> <499ecf26-0f57-2376-a617-9b6214319b4a@simark.ca> <87r0yx6sr7.fsf@redhat.com> <3ac49ef4-4bcf-2abc-4e06-0c9f3b8c9991@simark.ca> Date: Thu, 27 Oct 2022 11:38:44 +0100 Message-ID: <87edut5z63.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.8 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: Simon Marchi writes: > On 10/24/22 13:22, Andrew Burgess via Gdb-patches wrote: >>> Can we make gdbpy_disassembler::read_memory_func "noexcept", so that if >>> an exception escapes that function, regardless of the architecture, >>> there will be an apparent GDB crash? >>=20 >> Yes! This was my first thought once I spotted this bug. >>=20 >> So, what I originally tried was this, in gdb/disasm.h, I applied this: >>=20 >> diff --git a/gdb/disasm.h b/gdb/disasm.h >> index b7d16744c20..7a217dfad12 100644 >> --- a/gdb/disasm.h >> +++ b/gdb/disasm.h >> @@ -52,7 +52,9 @@ struct gdb_disassemble_info >> protected: >> =20 >> /* Types for the function callbacks within m_di. */ >> - using read_memory_ftype =3D decltype (disassemble_info::read_memory_f= unc); >> + using read_memory_ftype >> + =3D int (*) (bfd_vma, bfd_byte *, unsigned int, >> + struct disassemble_info *) noexcept; >> using memory_error_ftype =3D decltype (disassemble_info::memory_error= _func); >> using print_address_ftype =3D decltype (disassemble_info::print_addre= ss_func); >> using fprintf_ftype =3D decltype (disassemble_info::fprintf_func); >>=20 >> What I was trying to do was require that _every_ read memory function >> that we have in GDB _must_ be noexcept. This makes sense I think - the >> same issue I ran into here could just as easily crop up elsewhere if we >> tried to throw an exception. >>=20 >> Unfortunately, the above doesn't build, I run into this warning/error: >>=20 >> In file included from ../../src/gdb/disasm.c:25: >> ../../src/gdb/disasm.h: In constructor =E2=80=98gdb_printing_disassemb= ler::gdb_printing_disassembler(gdbarch*, ui_file*, gdb_disassemble_info::re= ad_memory_ftype, gdb_disassemble_info::memory_error_ftype, gdb_disassemble_= info::print_address_ftype)=E2=80=99: >> ../../src/gdb/disasm.h:116:3: error: mangled name for =E2=80=98gdb_pri= nting_disassembler::gdb_printing_disassembler(gdbarch*, ui_file*, gdb_disas= semble_info::read_memory_ftype, gdb_disassemble_info::memory_error_ftype, g= db_disassemble_info::print_address_ftype)=E2=80=99 will change in C++17 bec= ause the exception specification is part of a function type [-Werror=3Dnoex= cept-type] >> 116 | gdb_printing_disassembler (struct gdbarch *gdbarch, >> | ^~~~~~~~~~~~~~~~~~~~~~~~~ >>=20 >> At this point I figured I couldn't use noexcept in the API like this, so >> gave up. > > I think this warning could safely be silenced, since we don't care about > the mangling name change. > >> But I think this was a mistake. I just tried, and turns out I can add >> noexcept to gdbpy_disassembler::read_memory_func, and everything >> compiles fine, which, thinking about it more makes perfect sense. The >> noexcept function is more restrictive, so passing it to a function >> pointer type that doesn't include noexcept can't hurt. > > That's what I tried to, but your idea of putting it on read_memory_ftype > would be stronger, if it's possible. > >> What I'd like to do though it create a single patch that adds noexcept >> to all the disassembler callbacks throughout GDB in one go. I'll reply >> to this message once I have that patch ready, but if it's OK, I'll leave >> that as a separate change to the original patch? > > Yeah sure. > >>> >>>> >>>> This commit proposes an alternative solution that does not rely on >>>> throwing a C++ exception. >>>> >>>> When we spot an unhandled Python exception in frame #0, we will store >>>> the details of this exception within the gdbpy_disassembler object >>>> currently in use. Then we return to libopcodes a value indicating >>>> that the memory_read failed. >>>> >>>> libopcodes will now continue to disassemble as though that memory read >>>> failed (with one special case described below), then, when we >>>> eventually return to disasmpy_builtin_disassemble we check to see if >>>> there is an exception stored in the gdbpy_disassembler object. If >>>> there is then this exception can immediately be installed, and then we >>>> return back to Python, when the user will be able to catch the >>>> exception. >>>> >>>> There is one extra change in gdbpy_disassembler::read_memory_func. >>>> After the first call that results in an exception being stored on the >>>> gdbpy_disassembler object, any future calls to the ::read_memory_func >>>> function will immediately return as if the read failed. This avoids >>>> any additional calls into user supplied Python code. >>>> >>>> My thinking here is that should the first call fail with some unknown >>>> error, GDB should not keep trying with any additional calls. This >>>> maintains the illusion that the exception raised from >>>> MyInfo.read_memory is immediately raised by >>>> gdb.disassembler.builtin_disassemble. I have no tests for this change >>>> though - to trigger this issue would rely on a libopcodes disassembler >>>> that will try to read further memory even after the first failed >>>> read. I'm not aware of any such disassembler that currently does >>>> this, but that doesn't mean such a disassembler couldn't exist in the >>>> future. >>> >>> I don't really understand the need for this change. The >>> read_memory_func interface should mostly be stateless, if you try to >>> read an invalid address, you get an error. If you then try to read a >>> valid address, I don't see why that wouldn't work. >>=20 >> If the error/exception that occurs the first time is a gdb.MemoryError >> then I agree with you completely. >>=20 >> The question is about any other exception type. Imagine this totally >> made up nonsense example: >>=20 >> class MyInfo(gdb.disassembler.DisassembleInfo): >> def __init__(self, info): >> super().__init__(info) >>=20 >> def read_memory(self, length, offset): >> if read_resource_from_somewhere_remote(): >> return super().read_memory(length, offset) >> else: >> raise RemoteResourceUnavailableError() >>=20 >> If the disassembler tries to read memory, and for some reason the >> read_resource_from_somewhere_remote() call fails, we raise a >> RemoteResourceUnavailableError exception. >>=20 >> My expectation (and my desire for the API) is that the >> RemoteResourceUnavailableError will _immediately_ be re-raised by the >> corresponding builtin_disassemble call. >>=20 >> Without the line you are commenting on though, it is possible that the >> disassembler will repeatedly call the read_memory method, and so >> repeatedly raise the RemoteResourceUnavailableError error. >>=20 >> I don't know if I'm doing a good job of arguing my position :/ does the >> above make any sense? > > I don't really understand the problem with raising > RemoteResourceUnavailableError repeatedly, if the disassembler calls > read_memory repeatedly. > > Ah, I might get it now that I read your comment below. Is that you have > these stack frames: > > 0. gdb Python read_memory stack frames > 1. opcodes disassembler stack frames > 2. other gdb stack frames More like: 0. User Python code for read_memory, 1. gdb frame that calls into Python read_memory, 2. libopcodes disassembler stack frames, 3. gdb frame that implemented Python builtin_disassemble call, 4. User Python code that calls builtin_disassemble, 5. gdb frame that calls into Python disassembler, 6. other gdb stack frames. What used to happen: - Python exception raised in frame 0, - Python exception converted to C++ exception in 1, and thrown, - C++ exception caught in 3 and converted back to a Python exception, - Python exception appears in frame 4 for the user to handle. Now what happens: - Python exception raised in frame 0, - Python exception stored in frame 1, control returns to frame 2 with an error code, - Control might re-enter frame 1 again, but the stored exception means we immediately return with an error code, - Eventually the disassembler gives up and returns to frame 3, - We spot the stored exception and reinstall it in frame 3, - Python exception appears in frame 4 for the user to handle. This gives the _illusion_ to the user that the exception raised in read_memory immediately propagated up the stack and was raised as if from the builtin_disassemble call. Currently (thought this could change as this stuff has not yet been released) the documentation says that the _only_ exception frame 1/2 know how to handle is gdb.MemoryError. Anything else is not handled by the disassembler. If we allow frame 2 to keep calling to frame 1/0 even after an unknown exception has been raised then this just feels weird to me. Hopefully the user code will just keep raising the same, or similar exception each time. If it doesn't though, then there's the possibility that the disassembler results will be undefined. Like you say, it gives the _impression_ that the disassembler has handled the exception ... but it really hasn't. > > The Python exception is stored in frames 0, and normally restored in > frames 2. However, it could be that frames 0 store an exception, then > control goes back to frames 1, which handle the error return code, but > somehow think they can recover and so continue and call read_memory > again. And so we in frames 0 again, but the packaged Python exception > from last time is still there. > > If so, I think it would be fine to drop the original exception upon > entering frames 0 again. It would be as if frames 1 caught the > exception (if it were a C++ exception) and decided it was safe to > ignore. It would be surprising thought for a disassembler to do that. I agree that it would be surprising for a disassembler to claim to handle an error that it has, in fact, not handled. > After getting an error that can't be explained, you'd think they would > return immediately with an error. Remember, libopcodes doesn't have a concept of "unexplained" errors. There's only one error type in libopcode, memory errors. This whole mechanism is me attempting to implement the concept of "unexplained" errors on top of libopcodes. Initially I did this by bypassing libopcodes completely, but that doesn't work because libopcode is C code. So now, I'm trying to overload the memory-error mechanism, i.e. store the unexplained error, then return the memory-error value. In most cases, the disassembler will return as soon as it sees the first memory-error, so for (I'm guessing) 99% of cases, this whole discussion is moot. I'm just keen to lock down the actual behaviour and not leave it at the whim of the disassembler, just in case we hit that 1% case. I'll take another pass at patch 2/2 and see if I can silence the compiler warning about using noexcept in the function type. Thanks for your continued reviews, Andrew