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 04BE13844769 for ; Fri, 10 May 2024 13:31:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 04BE13844769 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 04BE13844769 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715347872; cv=none; b=Se+t61tyLO7DzTCZXwGGyFtrSBgJS/5EQYeewAhP4t/WpaH7EaxtVAgOu1M/IqOV2LyLVaeJ+uSq116qt0akLv97SX2MhncKxUQe5V+J+t3q0RJwNuXnjsv7nn6vt2TxIoYllorsZWSdlZTLLh6Gk1xPx4NUIyTsUAobU4erPXw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715347872; c=relaxed/simple; bh=qnI7vZnIIMOr7cKpes0qEFjtWQgHvALdTLLCjzRCN48=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=cWMrvaeve04uh9Azo7EMSMKHEIR40O8zmg5Q+qVdnIyETQr09mIwHa1V309dwyrddyC4DyHFv1HtUbuXzKlnLwqnyZJHQGoprAQ2KvcIi6Kxo1Iu6HgnSrg+fas8D35SnnGZc8CQW/KLnI/3S9Y1AZzp9M1reXrBkUPlPfdZ5lk= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1715347869; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=fzn6CSBhDfncHYnR8fKDPWKFvGjk78N3ehgsseRyPxU=; b=U04xlRywS1NUbCrg0TAzkcoQ3kxceFr7n7JkdVmBn1OvEvMGcQfj6PniKWT2gNkDv/y+1n 59VLmNUcg/JetWO8GopRptfcq1wEbbsL4ITGKsL4A+dlTfgkPvgML1RP+Mb3tgNXLK4OBz 4nDVbvdzP4OqsAC4IwdltaHH10IRox8= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-315-i7iz-ZacNrGnv0e7L62OCw-1; Fri, 10 May 2024 09:31:08 -0400 X-MC-Unique: i7iz-ZacNrGnv0e7L62OCw-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-41fda32e6c0so8007165e9.1 for ; Fri, 10 May 2024 06:31:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715347866; x=1715952666; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=fzn6CSBhDfncHYnR8fKDPWKFvGjk78N3ehgsseRyPxU=; b=geHUMlHK294l7IECJB6d1rQ5I2LxXa7y57+mdsTNn0NH/kBUUT/eQd5NNaxV/DhANd IsBM1Y6B6pGQV+04HO4x141GO59nf48oLyyOtlYzWikNN8tRUB5P7mry/KIGw1jkD4Fe jJ9Nf0MAEOO3sev6+tkI2dySpya5gVbD3RKX2GcPlcWjo5T6+dHTGCXgxv8umvPlf8/4 BKma/OYFq717hdYae8lKZPJ56Us71SQeV8x8uVoo+WfxqjSXBa3VdYQ+kodVaBI8ufSh H4UKvb1Tv8Ov1fyMoukU8yYFFZFL7u057EbWOXxP3SroB+jU7aBMsu6KnaGl58AlyEB6 /8Lg== X-Forwarded-Encrypted: i=1; AJvYcCWmDHr1P/BFdzL3UynanBN1Q/ZWQthroBWqxpWmfYWB/YwnZhopbYfGRFGHMC7qZX0VlOneKD5DEjecY8Cs5cv+xwxgThOulgOvSw== X-Gm-Message-State: AOJu0YzYAJ0RKOXrArDsDTIgD0y1R8S1f1mVWPIEEIWvBeLS4xYr6TuB 1cmtzgCf8jhJ3npp4wcsO7SE1sVOLssC8+CXWUuAhFBLWHlNMyGsbKz6aauhqAd6qUjX5S7CiqN tCs5iJC6Fs0xqZID7LEFlstdi/kqRvTCh83j4953BnVoumSnR1vUIcNIB0Z5y04XarrU= X-Received: by 2002:a05:600c:35c7:b0:41c:2334:fffd with SMTP id 5b1f17b1804b1-41feaa39a37mr26307525e9.9.1715347866186; Fri, 10 May 2024 06:31:06 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEFSoIu9zweUHX5QKQxo11iesC4N9grp7vrEN5kdxo92bAuE5yNcQzvsT/RIIoLoUGL6ix6WA== X-Received: by 2002:a05:600c:35c7:b0:41c:2334:fffd with SMTP id 5b1f17b1804b1-41feaa39a37mr26307095e9.9.1715347865518; Fri, 10 May 2024 06:31:05 -0700 (PDT) Received: from localhost ([31.111.84.240]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-41fccbe8e3csm64772495e9.1.2024.05.10.06.31.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 May 2024 06:31:05 -0700 (PDT) From: Andrew Burgess To: Tom Tromey Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: add gdbarch_stack_grows_down function In-Reply-To: <87ikzmq9xl.fsf@tromey.com> References: <874jb63h5s.fsf@tromey.com> <87bk5erqrb.fsf@redhat.com> <87ikzmq9xl.fsf@tromey.com> Date: Fri, 10 May 2024 14:31:04 +0100 Message-ID: <87wmo1rdyf.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.0 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_H4,RCVD_IN_MSPIKE_WL,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: Tom Tromey writes: >>>>>> "Andrew" == Andrew Burgess writes: > > Andrew> Great idea. How about the update below? > > Andrew> + /* We don't call gdbarch_stack_grows_down here, instead we're testing the > Andrew> + implementation by calling gdbarch_inner_than. GDB assumes that stacks > Andrew> + either grow down or up (see uses of gdbarch_stack_grows_down), so one of > Andrew> + these needs to be true. */ > Andrew> + bool stack_grows = (gdbarch_inner_than (gdbarch, 1, 2) > Andrew> + || gdbarch_inner_than (gdbarch, 2, 1)); > > It probably should check > > (gdbarch_inner_than() != 0) != (gdbarch_inner_than() != 0) > > to ensure that exactly one call returns true; with the !=0 being needed > because this still returns int and not bool. > > Maybe that's too nit-picky though. TBH I doubt it would ever be an > issue as is. Please, pick those nits! I updated the patch inline with your suggestion, double check the selftest still passes, and pushed the patch below. Thanks, Andrew --- commit a4f76c0765a0b9c643dc91d5a398a1cd9519572b Author: Andrew Burgess Date: Sun May 5 11:00:04 2024 +0100 gdb: add gdbarch_stack_grows_down function In another patch I'm working on I needed to ask: does the stack grow down, or grow up? Looking around I found in infcall.c some code where we needed to ask the same question, what we do there is ask: gdbarch_inner_than (gdbarch, 1, 2) which should do the job. However, I don't particularly like copying this, it feels like we're asking something slightly different that just happens to align with the question we're actually asking. I propose adding a new function `gdbarch_stack_grows_down`. This is not going to be a gdbarch method that can be overridden, instead, this will just call the gdbarch_inner_than function. We already have some gdbarch methods like this, checkout arch-utils.c for examples. I think it's now clearer what we're actually doing. A new self-test ensures that all architectures have a stack that either grows down, or grows up. There should be no user visible changes after this commit. Approved-By: Tom Tromey diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c index 0dc0c500654..db99fe08141 100644 --- a/gdb/gdbarch-selftests.c +++ b/gdb/gdbarch-selftests.c @@ -164,6 +164,21 @@ register_name_test (struct gdbarch *gdbarch) } } +/* Test gdbarch_stack_grows_down. Stacks must either grow down or up. */ + +static void +check_stack_growth (struct gdbarch *gdbarch) +{ + /* We don't call gdbarch_stack_grows_down here, instead we're testing the + implementation by calling gdbarch_inner_than. GDB assumes that stacks + either grow down or up (see uses of gdbarch_stack_grows_down), so exactly + one of these needs to be true. */ + bool stack_grows_down = gdbarch_inner_than (gdbarch, 1, 2) != 0; + bool stack_grows_up = gdbarch_inner_than (gdbarch, 2, 1) != 0; + + SELF_CHECK (stack_grows_up != stack_grows_down); +} + } // namespace selftests void _initialize_gdbarch_selftests (); @@ -175,4 +190,7 @@ _initialize_gdbarch_selftests () selftests::register_test_foreach_arch ("register_name", selftests::register_name_test); + + selftests::register_test_foreach_arch ("stack_growth", + selftests::check_stack_growth); } diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index 77d3406779f..d4c6795a12b 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -370,4 +370,12 @@ gdbarch_num_cooked_regs (gdbarch *arch) return gdbarch_num_regs (arch) + gdbarch_num_pseudo_regs (arch); } +/* Return true if stacks for ARCH grow down, otherwise return true. */ + +static inline bool +gdbarch_stack_grows_down (gdbarch *arch) +{ + return gdbarch_inner_than (arch, 1, 2) != 0; +} + #endif diff --git a/gdb/infcall.c b/gdb/infcall.c index 23d5652dd21..edac9a74179 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -947,7 +947,7 @@ reserve_stack_space (const type *values_type, CORE_ADDR &sp) struct gdbarch *gdbarch = get_frame_arch (frame); CORE_ADDR addr = 0; - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { /* Stack grows downward. Align STRUCT_ADDR and SP after making space. */ @@ -1128,7 +1128,7 @@ call_function_by_hand_dummy (struct value *function, address. AMD64 called that region the "red zone". Skip at least the "red zone" size before allocating any space on the stack. */ - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) sp -= gdbarch_frame_red_zone_size (gdbarch); else sp += gdbarch_frame_red_zone_size (gdbarch); @@ -1156,11 +1156,9 @@ call_function_by_hand_dummy (struct value *function, to pay :-). */ if (sp == old_sp) { - if (gdbarch_inner_than (gdbarch, 1, 2)) - /* Stack grows down. */ + if (gdbarch_stack_grows_down (gdbarch)) sp = gdbarch_frame_align (gdbarch, old_sp - 1); else - /* Stack grows up. */ sp = gdbarch_frame_align (gdbarch, old_sp + 1); } /* SP may have underflown address zero here from OLD_SP. Memory access @@ -1193,7 +1191,7 @@ call_function_by_hand_dummy (struct value *function, { CORE_ADDR lastval_addr = lastval->address (); - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { gdb_assert (sp >= lastval_addr); sp = lastval_addr;