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 9EF21385C30B for ; Fri, 10 Jun 2022 15:50:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9EF21385C30B 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.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-590-zzU2FrVDPTSrrSP8Co5I9g-1; Fri, 10 Jun 2022 11:50:02 -0400 X-MC-Unique: zzU2FrVDPTSrrSP8Co5I9g-1 Received: by mail-wm1-f69.google.com with SMTP id u12-20020a05600c19cc00b0038ec265155fso1474905wmq.6 for ; Fri, 10 Jun 2022 08:50:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=vqXuL18vCb5so48jlEs+ueNX2wgQ4TG172KQx9olLYI=; b=nGi4i2VyRYS52IC8l7HPSPLgv87QVKHjYkHtwlE/yWfvZWGgU7RbxYBs/VFKUV51hc j9nW/4mzSJ8TKiTWATDEr/fqHVJ1R2qLHMtOv3gDeryGfBPIECuoxYrtmggUWKnDR52f yo4ol7CmEj+YEW3Jl8SbfztSIPCz8Wc4zEWlJb4oydKWP4mqlncQudJQZ9CG5E1QIYUp UgQ/8HoTyheAkK6rg5p9ULJUDRiFDWQG7p/iPHTX4V7BNpXecCfylWlTWs5wevu3IUsZ txuwdONd4LZatSgVbfeq92Zk/2oqbL9+rRd6O65tQWkOISmo130rct+AbeuNeW0lax0F bJpg== X-Gm-Message-State: AOAM531yv6oRekBVLhMdwcCwMo+Nj76Gfk+HszeXlh5WDjZCxqdyCO5A 4rDi5rdPj1ukoT17FosQbrAkHqgN9in5vFcARl4+W8UIVa7PEDVrlzSc7fPxWl+A0XDXqxTjmnT oQbncpQwslY0cudVM3ZqPkA== X-Received: by 2002:adf:e28a:0:b0:210:b31:722 with SMTP id v10-20020adfe28a000000b002100b310722mr45173891wri.65.1654876201119; Fri, 10 Jun 2022 08:50:01 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxYYLWbI4Dba+Cm2/4Y8aGSTZZToApVZL24uHBuluaOkVttU/YkkkkWvdLSF7pKTKbUolCOPA== X-Received: by 2002:adf:e28a:0:b0:210:b31:722 with SMTP id v10-20020adfe28a000000b002100b310722mr45173872wri.65.1654876200808; Fri, 10 Jun 2022 08:50:00 -0700 (PDT) Received: from localhost (host109-152-215-36.range109-152.btcentralplus.com. [109.152.215.36]) by smtp.gmail.com with ESMTPSA id o19-20020a05600c4fd300b0039c55bc2c97sm3784089wmq.16.2022.06.10.08.50.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Jun 2022 08:50:00 -0700 (PDT) From: Andrew Burgess To: Luis Machado , gdb-patches@sourceware.org Subject: Re: [PATCHv2 3/6] gdb/arm: avoid undefined behaviour in arm_frame_is_thumb In-Reply-To: References: <0d968da223ab233af5ce95520f5472a4d849d269.1654866187.git.aburgess@redhat.com> Date: Fri, 10 Jun 2022 16:49:59 +0100 Message-ID: <87zgikecwo.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=-4.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_PORT autolearn=no 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: , X-List-Received-Date: Fri, 10 Jun 2022 15:50:06 -0000 Luis Machado via Gdb-patches writes: > On 6/10/22 14:08, Andrew Burgess via Gdb-patches wrote: >> This commit fixes real undefined behaviour in GDB which I spotted when >> working on a later patch in this series. The later patch in this >> series detects when the result of gdbarch_tdep() is cast to the wrong >> type. >> >> The issue is revealed by the gdb.multi/multi-arch.exp test. >> >> In this test we setup two inferiors, an AArch64 process, and an ARM >> process, then at one point we have inferior 1 selected (the AArch64 >> inferior), and we place a breakpoint on a symbol present in the other >> inferior (the ARM inferior). >> >> During the process of creating the breakpoint we call arm_pc_is_thumb, >> the GDBARCH passed into this function is correct, that is, represents >> the ARM process. >> >> For whatever reason we are unable to figure out if the address in >> question is thumb or not throughout most of arm_pc_is_thumb, and so we >> get to this code at the end of the function: >> >> /* If we couldn't find any symbol, but we're talking to a running >> target, then trust the current value of $cpsr. This lets >> "display/i $pc" always show the correct mode (though if there is >> a symbol table we will not reach here, so it still may not be >> displayed in the mode it will be executed). */ >> if (target_has_registers ()) >> return arm_frame_is_thumb (get_current_frame ()); >> >> Which I guess is a last attempt to figure out the thumb status of an >> address. However, remember, we the AArch64 inferior is current at >> this time, so the current frame is an AArch64 frame. > > If we're trying to insert a breakpoint into a 32-bit inferior, > we should really have the 32-bit arm gdbarch at hand, not the AArch64 > gdbarch. We do. > > I think the bug is somewhere else, in whoever passed the current inferior's gdbarch > as opposed to the gdbarch of the inferior that contains the symbol > we've found. We do pass in the gdbarch of the inferior containing the breakpoint location. If I change the condition to an assert, then run gdb.multi/multi-arch.exp and catch the assertion, the stack looks like this: #0 internal_error (file=0x55834840c8 "../../src/gdb/arm-tdep.c", line=551, fmt=0x5583483d70 "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:51 #1 0x0000005582b0e6a4 in arm_frame_is_thumb (frame=0x55c0849a00) at ../../src/gdb/arm-tdep.c:551 #2 0x0000005582b0eb70 in arm_pc_is_thumb (gdbarch=0x55c0937080, memaddr=4195692) at ../../src/gdb/arm-tdep.c:687 #3 0x0000005582b17e70 in arm_adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692) at ../../src/gdb/arm-tdep.c:4925 #4 0x0000005582af0a60 in gdbarch_adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692) at ../../src/gdb/gdbarch.c:2840 #5 0x0000005582b73c90 in adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692, bptype=bp_breakpoint) at ../../src/gdb/breakpoint.c:7147 #6 0x0000005582b76854 in code_breakpoint::add_location (this=0x55c088e340, sal=...) at ../../src/gdb/breakpoint.c:8100 #7 0x0000005582b773b8 in code_breakpoint::code_breakpoint (this=0x55c088e340, gdbarch_=0x55c089c050, type_=bp_breakpoint, sals=..., location_=..., filter_=std::unique_ptr = {...}, cond_string_=std::unique_ptr = {...}, extra_string_=std::unique_ptr = {...}, disposition_=disp_donttouch, thread_=-1, task_=0, ignore_count_=0, from_tty=1, enabled_=1, flags=0, display_canonical_=0) at ../../src/gdb/breakpoint.c:8329 #8 0x0000005582b90c00 in ordinary_breakpoint::code_breakpoint (this=0x55c088e340) at ../../src/gdb/breakpoint.c:266 #9 0x0000005582b88758 in new_breakpoint_from_type&, std::unique_ptr, std::unique_ptr >, std::unique_ptr >, std::unique_ptr >, bpdisp&, int&, int&, int&, int&, int&, unsigned int&, int&> (gdbarch=0x55c089c050, type=bp_breakpoint) at ../../src/gdb/breakpoint.c:1303 #10 0x0000005582b77710 in create_breakpoint_sal (gdbarch=0x55c089c050, sals=..., location=..., filter=std::unique_ptr = {...}, cond_string=std::unique_ptr = {...}, extra_string=std::unique_ptr = {...}, type=bp_breakpoint, disposition=disp_donttouch, thread=-1, task=0, ignore_count=0, from_tty=1, enabled=1, internal=0, flags=0, display_canonical=0) at ../../src/gdb/breakpoint.c:8395 #11 0x0000005582b779d8 in create_breakpoints_sal (gdbarch=0x55c089c050, canonical=0x7fd8d196c0, cond_string=std::unique_ptr = {...}, extra_string=std::unique_ptr = {...}, type=bp_breakpoint, disposition=disp_donttouch, thread=-1, task=0, ignore_count=0, from_tty=1, enabled=1, internal=0, flags=0) at ../../src/gdb/breakpoint.c:8438 #12 0x0000005582b78d94 in create_breakpoint (gdbarch=0x55c089c050, location=0x55c0979750, cond_string=0x0, thread=-1, extra_string=0x0, force_condition=false, parse_extra=1, tempflag=0, type_wanted=bp_breakpoint, ignore_count=0, pending_break_support=AUTO_BOOLEAN_AUTO, ops=0x558389c650 , from_tty=1, enabled=1, internal=0, flags=0) at ../../src/gdb/breakpoint.c:8923 #13 0x0000005582b792d8 in break_command_1 (arg=0x55c0732fc2 "", flag=0, from_tty=1) at ../../src/gdb/breakpoint.c:8994 #14 0x0000005582b79578 in break_command (arg=0x55c0732fb6 "hangout_loop", from_tty=1) at ../../src/gdb/breakpoint.c:9065 In frame #6 we select a gdbarch based on the location of the breakpoint, its at this point that we select the bfd_arch_arm gdbarch. For frames #5, #4, #3, and #2 we are passing in a bfd_arch_arm gdbarch, which is correct, and what you are asking for. The problem is that in frame #2 we fail to find any of the special hints that indicate if the code is thumb or not. Now, _maybe_ you could argue that one of the conditions in arm_pc_is_thumb should trigger, but, for me, the very fact that there is a "catch all" case at the end of the function means we have to be open to the possibility that non of the special symbols, or bottom bit of the address set cases might trigger. And so, we get to this code in arm_pc_is_thumb: if (target_has_registers ()) return arm_frame_is_thumb (get_current_frame ()); At this point GDBARCH _is_ a bfd_arch_arm architecture. But, the current frame is bfd_arch_aarch64. And so, we enter frame #1, arm_frame_is_thumb, passing in a frame that is not bfd_arch_arm. So, are you are suggesting we should switch frames as part of the breakpoint setting process? Because I'm not sure how you'd pick even a suitable inferior, multiple ARM inferiors might share a single program space, so we could have a single gdbarch, but multiple inferiors, each with multiple threads, and each thread with multiple frames... I guess we could push the architecture check out of arm_frame_is_thumb back to arm_pc_is_thumb, and only call arm_frame_is_thumb for the case where the architecture is ARM... that doesn't make sense to me, but maybe, I guess... Anyway, let me know if the above makes any more sense. If it does I can update the commit message. Thanks, Andrew