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 DAC0C3857806 for ; Mon, 27 Jun 2022 16:27:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DAC0C3857806 Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-317-WI2eNBRrND2GPeo9QSc2Rw-1; Mon, 27 Jun 2022 12:27:08 -0400 X-MC-Unique: WI2eNBRrND2GPeo9QSc2Rw-1 Received: by mail-wm1-f72.google.com with SMTP id h125-20020a1c2183000000b003a0374f1eb8so6426730wmh.8 for ; Mon, 27 Jun 2022 09:27:07 -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=u99C/TcpaHBkODesV2Z6+nNSac9w+9T2iQAuIb0STbU=; b=iMvinr/Pe/p/d1ngBLWmTpx4sV0jymtz8R6XwX0YFqwLttvD1yjEH2LnOJvN1ZG2qK pk813jiZ4QfA0QnVQodvchs+0f7S22ebh4sEQ6YPGwKHrogNpNZqDMFyUhnSInvxIKWd 3jvYeZ0/wdaU5hewmqXFGhFnvxPc7OY1Sm6o0MK+YvZABdWTTtOQxSbH8RYGzqe5KntQ YzV2BwLG1ENtwvTzJjlD5oXpD4pR+lTBIJ9BMzI8iSdWAIMC+dIDMPGn4cePhYSkC8Hj szprtOEzVn7iukQzH5ERIM/S7OvKAxSZS89NtcN9nQg7o7wbFLFAI9L85boR0MYFXTPt J34w== X-Gm-Message-State: AJIora8weu8JuZOM9ZfFjsRABt6pzcFtMMVQBet7KJ2KveExJNskFU4K ZDzUOkIDrxdaOeRcVUzd73GMCw+9K2QHXuikUXfyVaCnPZ+mUgUkfXl5KgMd/vMOAipwz4cuzF8 AzXnOFDagl1tjzauyWfWkZg== X-Received: by 2002:a5d:48c8:0:b0:21b:826a:6f86 with SMTP id p8-20020a5d48c8000000b0021b826a6f86mr12940338wrs.220.1656347226315; Mon, 27 Jun 2022 09:27:06 -0700 (PDT) X-Google-Smtp-Source: AGRyM1vmxP9CPu00W0U1d/eVFJPpxqz3R9zAU0kcfTyHUoHYPJEboLRd22lWoo4+rPGW/B0DgQ+XRA== X-Received: by 2002:a5d:48c8:0:b0:21b:826a:6f86 with SMTP id p8-20020a5d48c8000000b0021b826a6f86mr12940314wrs.220.1656347225952; Mon, 27 Jun 2022 09:27:05 -0700 (PDT) Received: from localhost (15.72.115.87.dyn.plus.net. [87.115.72.15]) by smtp.gmail.com with ESMTPSA id j13-20020a05600c1c0d00b003a0484c069bsm7543708wms.41.2022.06.27.09.27.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Jun 2022 09:27:05 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection In-Reply-To: <48d211cc-96f3-8c84-5aec-7023054d45ac@palves.net> References: <83ceec06f74f4ce6a2dc8d794031fb233567ba6d.1655136816.git.aburgess@redhat.com> <48d211cc-96f3-8c84-5aec-7023054d45ac@palves.net> Date: Mon, 27 Jun 2022 17:27:03 +0100 Message-ID: <87fsjqdqco.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=-10.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_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_PORT 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: , X-List-Received-Date: Mon, 27 Jun 2022 16:27:12 -0000 Pedro Alves writes: > On 2022-06-13 17:15, Andrew Burgess via Gdb-patches wrote: > >> This leaves just one question, what about native targets that support >> multiple architectures? >> > > I have another question. I'm confused on how if e.g., on x86-64, you > load a RISC-V binary into GDB, and the try to run it, we don't end up > with architecture of the target description instead. The problem is that we end up trying to read the registers before we read the target description. Which I suspect you will say sounds wrong, and I agree. I removed the error() calls from this patch, and reran GDB, here's the stack at the point of failure: #0 internal_error (file=0x16bcfc8 "../../src/gdb/../gdbsupport/gdb-checked-static-cast.h", line=60, fmt=0x16bcf80 "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:54 #1 0x00000000004c4f4c in gdb::checked_static_cast (v=0x337b910) at ../../src/gdb/../gdbsupport/gdb-checked-static-cast.h:60 #2 0x00000000004d09c2 in gdbarch_tdep (gdbarch=0x3a4c630) at ../../src/gdb/gdbarch.h:167 #3 0x00000000004d059e in amd64_supply_fxsave (regcache=0x30f5f10, regnum=-1, fxsave=0x7fffffff9070) at ../../src/gdb/amd64-tdep.c:3387 #4 0x00000000004c5958 in amd64_linux_nat_target::fetch_registers (this=0x2d27140 , regcache=0x30f5f10, regnum=32) at ../../src/gdb/amd64-linux-nat.c:260 #5 0x0000000000dd74bd in target_fetch_registers (regcache=0x30f5f10, regno=32) at ../../src/gdb/target.c:3948 #6 0x0000000000bf2540 in regcache::raw_update (this=0x30f5f10, regnum=32) at ../../src/gdb/regcache.c:587 #7 0x0000000000bf25e5 in readable_regcache::raw_read (this=0x30f5f10, regnum=32, buf=0x7fffffff9c60 "\200\234\377\377\377\177") at ../../src/gdb/regcache.c:601 #8 0x0000000000bf291d in readable_regcache::cooked_read (this=0x30f5f10, regnum=32, buf=0x7fffffff9c60 "\200\234\377\377\377\177") at ../../src/gdb/regcache.c:690 #9 0x0000000000bf9d6a in readable_regcache::cooked_read (this=0x30f5f10, regnum=32, val=0x7fffffff9d28) at ../../src/gdb/regcache.c:775 #10 0x0000000000bf2da9 in regcache_cooked_read_unsigned (regcache=0x30f5f10, regnum=32, val=0x7fffffff9d28) at ../../src/gdb/regcache.c:789 #11 0x0000000000bf43c4 in regcache_read_pc (regcache=0x30f5f10) at ../../src/gdb/regcache.c:1325 #12 0x00000000009c2e19 in save_stop_reason (lp=0x3a8ff80) at ../../src/gdb/linux-nat.c:2545 #13 0x00000000009c40f0 in linux_nat_filter_event (lwpid=3990223, status=1407) at ../../src/gdb/linux-nat.c:3003 #14 0x00000000009c471a in linux_nat_wait_1 (ptid=..., ourstatus=0x7fffffffa4f0, target_options=...) at ../../src/gdb/linux-nat.c:3178 #15 0x00000000009c535a in linux_nat_target::wait (this=0x2d27140 , ptid=..., ourstatus=0x7fffffffa4f0, target_options=...) at ../../src/gdb/linux-nat.c:3416 #16 0x0000000000dc5f3b in target_wait (ptid=..., status=0x7fffffffa4f0, options=...) at ../../src/gdb/target.c:2601 #17 0x0000000000abd3c4 in startup_inferior (proc_target=0x2d27140 , pid=3990223, ntraps=1, last_waitstatus=0x0, last_ptid=0x0) at ../../src/gdb/nat/fork-inferior.c:482 #18 0x00000000008a0165 in gdb_startup_inferior (pid=3990223, num_traps=1) at ../../src/gdb/fork-child.c:129 #19 0x0000000000936f6e in inf_ptrace_target::create_inferior (this=0x2d27140 , exec_file=0x337c1b0 "/tmp/hello.rv32imc.x", allargs=..., env=0x2dba940, from_tty=1) at ../../src/gdb/inf-ptrace.c:105 #20 0x00000000009bef5b in linux_nat_target::create_inferior (this=0x2d27140 , exec_file=0x337c1b0 "/tmp/hello.rv32imc.x", allargs=..., env=0x2dba940, from_tty=1) at ../../src/gdb/linux-nat.c:977 #21 0x00000000009424ee in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../src/gdb/infcmd.c:461 #22 0x0000000000942784 in run_command (args=0x0, from_tty=1) at ../../src/gdb/infcmd.c:519 In contrast, the call to target_find_description is done (indirectly) from later in run_command_1, like this: #0 target_find_description () at ../../src.dev-4/gdb/target-descriptions.c:521 #1 0x0000000000941e7c in post_create_inferior (from_tty=0) at ../../src.dev-4/gdb/infcmd.c:253 #2 0x00000000009425ea in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../src.dev-4/gdb/infcmd.c:494 #3 0x0000000000942784 in run_command (args=0x0, from_tty=1) at ../../src.dev-4/gdb/infcmd.c:519 So, after I start GDB like: $ arch x86_64 $ gdb -q /tmp/hello.rv32imc.x Reading symbols from /tmp/hello.rv32imc.x... (gdb) show arch The target architecture is set to "auto" (currently "riscv:rv32"). (gdb) run ... The initial architecture is riscv:rv32, and it is this architecture that is used for handling the initial stop. So my first thought was, pretty much as you suggest, that we should fetch the target description earlier, then we'll not run into this problem at all. My problem was figuring out the right place to add such a call. We know it has to be before we enter regcache_read_pc, as at this point the regcache is already created, and has the wrong architecture. And it can't really be earlier than linux_nat_filter_event I think, as before this point we don't know that the target has stopped. We can't probe the target for features (I would assume) if the target is not currently stopped. This seems to leave either save_stop_reason, or in the early stages of get_thread_regcache maybe. The problem with save_stop_reason is that it's Linux specific, so any non-Linux targets will need to add their own code to catch this situation. Inside the get_thread_regcache code is possible I guess, but something about it seemed off, I guess it was the idea of calling get_thread_regcache and having the thread's architecture change under your feet - that seemed pretty unexpected to me. > If the target > reports a target description, and the resulting architecture is not > compatible with the executable, then I think it would make sense to > use the architecture of the target? I agree. For a native target once the target actually tries to start the executable I think we'd expect the target itself to report an error relating to the wrong architecture. > > If I start a 64-bit program on x86-64, like: > > $ gdb ~/gdb/tests/main64 > GNU gdb (GDB) 13.0.50.20220624-git > ... > (gdb) start > Temporary breakpoint 1 at 0x400492: file main.c, line 11. > Starting program: /home/pedro/gdb/tests/main64 > > Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at main.c:11 > 11 return 0; > > and then try to load the executable of an incompatible arch, like: > > (gdb) file ~/gdb/tests/main32 > A program is being debugged already. > Are you sure you want to change the file? (y or n) y > warning: Selected architecture i386 is not compatible with reported target architecture i386:x86-64 > Architecture of file not recognized. > > then GDB doesn't switch the architecture to the file's architecture, it is still x86-64: > > (gdb) show architecture > The target architecture is set to "auto" (currently "i386:x86-64"). > > and it doesn't let me manually select an incompatible architecture either: > > (gdb) set architecture riscv > warning: Selected architecture riscv is not compatible with reported target architecture i386:x86-64 > Architecture `riscv' not recognized. [ Unrelated: We should probably remove that 'Architecture `riscv' not recognized.' line, as that seems wrong here. The architecture is recognized, it's just not compatible.] > The target architecture is set to "auto" (currently "i386:x86-64"). > (gdb) > > It kind of sounds like if GDB detected the incompatibility when we first fetch the > target description, similarly to how we detect it in the cases above, and GDB picked > the target arch or errored out after detecting the incompatibility, then we wouldn't > need a patch like yours? I agree. One other aspect is, do we have any native targets that don't support reading a target description? Hopefully not. > How come the mismatch isn't detected then (when we first > retrieve the target description) today? Hopefully the above makes that clear, but the TLDR answer is: we attempt to read the registers using the current architecture before we fetch the target description. I suspected that, in part, this patch would be a bit of a straw-man, I'd appreciate any insight you have for how we might reorder things so that the target description can be read before we try to read the registers. Thanks, Andrew > >> + >> protected: >> /* Unpush the target if it wasn't explicitly open with "target native" >> and there are no live inferiors left. Note: if calling this as a >> diff --git a/gdb/infcmd.c b/gdb/infcmd.c >> index e909d4d4c81..b8acf858b3a 100644 >> --- a/gdb/infcmd.c >> +++ b/gdb/infcmd.c >> @@ -413,6 +413,10 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how) >> if (non_stop && !run_target->supports_non_stop ()) >> error (_("The target does not support running in non-stop mode.")); >> >> + if (!run_target->supports_architecture_p (get_current_arch ())) >> + error (_("The target does not support architecture \"%s\"."), >> + gdbarch_bfd_arch_info (get_current_arch ())->printable_name); >> + >> /* Done. Can now set breakpoints, change inferior args, etc. */ >> >> /* Insert temporary breakpoint in main function if requested. */ >> @@ -2590,6 +2594,10 @@ attach_command (const char *args, int from_tty) >> if (non_stop && !attach_target->supports_non_stop ()) >> error (_("Cannot attach to this target in non-stop mode")); >> >> + if (!attach_target->supports_architecture_p (get_current_arch ())) >> + error (_("The target does not support architecture \"%s\"."), >> + gdbarch_bfd_arch_info (get_current_arch ())->printable_name); >> + > > Since the error is the same in both cases, I'd suggest putting it in > a shared routine, like: > > static void > check_target_supports_current_arch (process_stratum_target *target) > { > if (!target->supports_architecture_p (get_current_arch ())) > error (_("The target does not support architecture \"%s\"."), > gdbarch_bfd_arch_info (get_current_arch ())->printable_name); > }