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 C5C7F3858C52 for ; Sat, 20 May 2023 09:19:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C5C7F3858C52 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=1684574379; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=pFOrLUvngyVqer9a6ZQ2TDNmLyezomC7V6zRbL8p33g=; b=ea/Q+fXatoaIMwU4QqlgSrbDLdT49K0B3ySK0CVIwDSnmLCMB0/4e3sadsnM7q78tg+J74 kZunKNCsg/6mHwC9irYEeXJOmd2PUFOtMiXSzdOIHDVRBhaT+erSSZMUs3RzVzvLf0er8w 1jIu9Aq5EIrdJIIiL2QgArWQnTOX9P4= 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.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-502-IrcpGohtOyedAXhLUhsrPQ-1; Sat, 20 May 2023 05:19:38 -0400 X-MC-Unique: IrcpGohtOyedAXhLUhsrPQ-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-3f50aa22cd2so25540555e9.1 for ; Sat, 20 May 2023 02:19:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684574377; x=1687166377; 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=pFOrLUvngyVqer9a6ZQ2TDNmLyezomC7V6zRbL8p33g=; b=F5ULtOh0HYCS1fgltA9q+qW03gEQ4PtYPuqpBNaKjklOC75th2ic0pxgy8jXER2FFT rUOjl6N8uowqW95n8Brtdhwyk93fOAT9jBl3KEnQfTLH2mmvP73G9CqExiKxZ+jRjwyO domhQRSSEWlhjdGASaDH7npshFV0kuP/8Fst3ClxJzm2tV1Si4VwbG+XzNyx2ZAxhpWP gGIxknaRzehRIf5kiDXZL3CiB47KEvCgYZimuIloh/6r3kVxXkz+hGK8QEzQ1FkGrBtX uGW73uIfBGl6nfbT84eTIyDJOkJ0iCO6DZpqFUd98d3DptZdKrM5rXf+Rh3HOXjAG/wF ooDQ== X-Gm-Message-State: AC+VfDzR5elAbye3rpUF31DjwbFt1wuAgvcg9tuVJBdVIyr1MtMTJUWc 8dx4qBeDyt/PzBxcUJbMmlBNn3lY+wRd8i1lehqlRff2WK8nCUJicbW9nHrHyD1GF4LbYR1McAK e8D0itrfnKqFqkn0N18AaobC5NmoTgmU++XUD68f5iFrETFizMi9rrlgMo9IXfukWatH7fww6Tg lk3XrPfA== X-Received: by 2002:a7b:c455:0:b0:3f5:60b:31 with SMTP id l21-20020a7bc455000000b003f5060b0031mr3073097wmi.27.1684574376727; Sat, 20 May 2023 02:19:36 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4/xYCDQluUYzKhYUHvMTMBzoIecqjT/RmZf1zd/XeyGAJ3ZHjkDpkJGdQF3TJejqUP0KO9zg== X-Received: by 2002:a7b:c455:0:b0:3f5:60b:31 with SMTP id l21-20020a7bc455000000b003f5060b0031mr3073075wmi.27.1684574376186; Sat, 20 May 2023 02:19:36 -0700 (PDT) Received: from localhost (92.40.219.223.threembb.co.uk. [92.40.219.223]) by smtp.gmail.com with ESMTPSA id k9-20020a05600c0b4900b003f42813b315sm4881881wmr.32.2023.05.20.02.19.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 May 2023 02:19:35 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen via Gdb-patches , Bruno Larsen , Bruno Larsen via Gdb-patches Subject: Re: [PINGv2][PATCH] gdb/testsuite: change hardcoded assembly in gdb.arch/disp-step-insn-reloc.exp In-Reply-To: References: <20230426132916.1988539-1-blarsen@redhat.com> Date: Sat, 20 May 2023 10:19:33 +0100 Message-ID: <87lehj8imy.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=-12.1 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,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 List-Id: Bruno Larsen via Gdb-patches writes: > Ping! > > -- > Cheers, > Bruno > > On 26/04/2023 15:29, Bruno Larsen wrote: >> When testing gdb.arch.disp-step-insn-reloc.exp with clang in an x86_64 >> machine, the compiled test case would segfault when returning from >> the function can_relocate_call, with a suggestion of a broken stack. >> The example assembly in the commment was the following: >> >> f: >> MOV $1, %[ok] >> JMP end >> set_point0: >> CALL f ; tracepoint here. >> end: >> >> And the segmentation fault happening at the final "ret" instruction of >> the original function. This suggests that gcc's compilation process would >> realize that no ret instruction ever happened after that call and doesn't >> save the return address, while clang's process wouldn't. Looking at the >> generated instructions, we can indeed see a difference: This replaces my earlier feedback. This description is not really correct. As far as I'm aware call always pushes the return address onto the stack, so it's not the call that changes (in a significant way) between GCC and Clang. Here's the Clang disassembly (without your patch): 0000000000001190 : 1190: 55 push %rbp 1191: 48 89 e5 mov %rsp,%rbp 1194: 48 83 ec 10 sub $0x10,%rsp 1198: c7 45 fc 00 00 00 00 movl $0x0,-0x4(%rbp) 119f: e9 0a 00 00 00 jmpq 11ae 11a4: b8 01 00 00 00 mov $0x1,%eax 11a9: e9 05 00 00 00 jmpq 11b3 00000000000011ae : 11ae: e8 f1 ff ff ff callq 11a4 11b3: 89 45 fc mov %eax,-0x4(%rbp) 11b6: 83 7d fc 01 cmpl $0x1,-0x4(%rbp) 11ba: 0f 85 0a 00 00 00 jne 11ca 11c0: e8 5b 00 00 00 callq 1220 11c5: e9 05 00 00 00 jmpq 11cf 11ca: e8 61 00 00 00 callq 1230 11cf: 48 83 c4 10 add $0x10,%rsp 11d3: 5d pop %rbp 11d4: c3 retq 11d5: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1) 11dc: 00 00 00 00 And here's GCC (also unpatched): 0000000000401114 : 401114: 55 push %rbp 401115: 48 89 e5 mov %rsp,%rbp 401118: 48 83 ec 10 sub $0x10,%rsp 40111c: c7 45 fc 00 00 00 00 movl $0x0,-0x4(%rbp) 401123: eb 07 jmp 40112c 401125: b8 01 00 00 00 mov $0x1,%eax 40112a: eb 05 jmp 401131 000000000040112c : 40112c: e8 f4 ff ff ff callq 401125 401131: 89 45 fc mov %eax,-0x4(%rbp) 401134: 83 7d fc 01 cmpl $0x1,-0x4(%rbp) 401138: 75 07 jne 401141 40113a: e8 c7 ff ff ff callq 401106 40113f: eb 05 jmp 401146 401141: e8 c7 ff ff ff callq 40110d 401146: 90 nop 401147: c9 leaveq 401148: c3 retq The critical difference is actually the very end of the function, starting at 0x11cf in Clang, and 0x401147 in GCC. For both builds, when we do the "inner" call -- the fake one which you correctly identify as missing a 'ret' -- we leave the return address within can_relocate_call pushed onto the stack, the $rsp value is 0x8 bytes offset from where can_relocate_call expects it to be. However, our fake "inner" function, doesn't adjust $rbp, so that register is still correctly set as can_relocate_call expects it. In the Clang build, when we return we manually adjust $rsp with an offset (so the new value is still 0x8 bytes off from where it should be), then pop $rbp, so we pop this from the wrong stack slot, and then we ret, which pulls the previous $pc value from the wrong stack slot. In the GCC build we just the leaveq instruction, this combines the stack adjust and the pop by just copying $rbp into $rsp. As Our fake "inner" function never adjusted $rbp, the $rbp value was still correct, so we restore the correct value of $rsp. Then when we retq we pull the previous $pc from the correct stack slot and all is good. The fix is, or course, exactly what you originally proposed -- change the fake "inner" function to end with a ret. But the above is why that works. Could you rewrite the commit message and repost this patch please. Thanks, Andrew >> >> clang's version: e8 f1 ff ff ff call 11a4 >> gcc's version: e8 f4 ff ff ff call 401125 >> >> Notice the difference on the second byte. >> >> Changing the assembly to use "ret" instead of "JMP end" does not change >> the behavior of the program and guarantees a compiler independent >> behavior. This commit does just that. >> --- >> gdb/testsuite/gdb.arch/insn-reloc.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/gdb/testsuite/gdb.arch/insn-reloc.c b/gdb/testsuite/gdb.arch/insn-reloc.c >> index f687c2c5631..365e6180057 100644 >> --- a/gdb/testsuite/gdb.arch/insn-reloc.c >> +++ b/gdb/testsuite/gdb.arch/insn-reloc.c >> @@ -49,10 +49,9 @@ fail (void) >> JMP set_point0 >> f: >> MOV $1, %[ok] >> - JMP end >> + RET >> set_point0: >> CALL f ; tracepoint here. >> - end: >> >> */ >> >> @@ -65,10 +64,9 @@ can_relocate_call (void) >> " jmp " SYMBOL (set_point0) "\n" >> "0:\n" >> " mov $1, %[ok]\n" >> - " jmp 1f\n" >> + " ret\n" >> SYMBOL (set_point0) ":\n" >> " call 0b\n" >> - "1:\n" >> : [ok] "=r" (ok)); >> >> if (ok == 1)