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 ESMTP id 074FB384400A for ; Fri, 26 Mar 2021 19:52:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 074FB384400A Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-314-wlzwlYR1N3OjHumkd4gJ2A-1; Fri, 26 Mar 2021 15:52:28 -0400 X-MC-Unique: wlzwlYR1N3OjHumkd4gJ2A-1 Received: by mail-qk1-f198.google.com with SMTP id b127so6947381qkf.19 for ; Fri, 26 Mar 2021 12:52:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=yVRKvKQyLoo1Qu0bVHi1vkhSIKnISuq6pey5PXDsdp0=; b=ZMS+iIj71d0et9XkOYqvkmccfdLdcLDJAz6MNCSiEHNsfya69TQ1xF5XCOd/ZBQQLK 6wIgs7CQg7CUVtw3OB2BD8DEjbwxwrvWNXKUHYzf55sZaCEw3zx9+q/FxoMOGceViEfz VWQD30d6NibZX4gLt9FqTC4LhFpWGxPh7VhrqnX8uZG/KeVHL8rsJksXdaCttMHXtYec oNWnF31IoTzBXUzciZmEQxxTp4C3KPCfezpvITcOsTjhLyBzd/Bbv1tNrVvzZlB2XGWO vw9xY+56vdNSUh+cRJVNK015VPmXMw4xvr7r2Pq0dj2UV+2ych83mfmGdjWBi37Kj/pr 0LBA== X-Gm-Message-State: AOAM531EAL31gmXP7Hj/GjapiAcNA+qO/rzyqK8qKsggP43ADlYvWC1Q vIyrPVt8hiBSVZ4om7uHMG+f38CZcixC71NAnr2BVzOrkVgrYye7pzmu/+ycbZGFoQhZSZNCnG5 QCy8JjuYXeK4= X-Received: by 2002:a37:ac18:: with SMTP id e24mr14999100qkm.30.1616788347487; Fri, 26 Mar 2021 12:52:27 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyTksxdQjAhUM67GqL0gH+XfJsq0F41/+8MHrZjZDkH3yPn/tvOWTQf2Up3TBYkTZgZ99rjoQ== X-Received: by 2002:a37:ac18:: with SMTP id e24mr14999077qkm.30.1616788347076; Fri, 26 Mar 2021 12:52:27 -0700 (PDT) Received: from [192.168.1.148] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id w5sm7400451qkc.85.2021.03.26.12.52.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 26 Mar 2021 12:52:26 -0700 (PDT) Subject: Re: [PATCH] dwarf2cfi: Defer queued register saves some more [PR99334] To: Jakub Jelinek Cc: Richard Henderson , gdb@sourceware.org, gcc-patches@gcc.gnu.org, Mark Wielaard References: <20210324093132.GE186063@tucnak> <0d60aeed-783d-1128-65e5-fc24c504bf20@redhat.com> <20210326172910.GX1179226@tucnak> From: Jason Merrill Message-ID: <47f18f14-e52c-2aa0-aad2-d307c2775746@redhat.com> Date: Fri, 26 Mar 2021 15:52:25 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <20210326172910.GX1179226@tucnak> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Mar 2021 19:52:32 -0000 On 3/26/21 1:29 PM, Jakub Jelinek wrote: > On Thu, Mar 25, 2021 at 05:43:57PM -0400, Jason Merrill via Gcc-patches wrote: >>> Will e.g. GDB be happy about the changes? >> >> I would expect so, but it would be good to have someone from GDB verify. > > Ok, have verified it now on the testcase from the PR (stubbed out what it > calls, called it from main with all NULL pointers and just in gdb stepped > through the prologue and at each insn looked at > p $rsp > p $rbp > up > p $rsp > p $rbp > down > stepi > With unpatched gcc indeed it works everywhere except when stopping > at the start of movq %rsp, %rbp instruction where $rbp in the parent > frame was unrelated garbage. > And with the patch below, I get the expected values in all cases. > >>> Thinking some more about this, what can be problematic on the GCC side >>> is inline asm, that can and often will contain multiple instructions. >>> For that is an easy fix, just test asm_noperands and handle >>> clobbers_queued_reg_save before the insn rather than after in that case. >> >> Sure, but does inline asm go through dwarf2out_frame_debug? > > Not through dwarf2out_frame_debug, as inline asm is not RTX_FRAME_RELATED_P, > but it goes through that scan_trace hunk I was changing like any other > insn (except for BARRIERs, instructions not in basic blocks, some NOTEs > and DEBUG_INSNs). And clobbers_queued_reg_save (insn) can be true for the > inline asm either because it stores some registers or because it clobbers > them. > And then there are other backend insns that expand to multiple hw > instructions, e.g. in the i386 backend typically (but not sure if we > guarantee that) marked as get_attr_type (insn) == TYPE_MULTI. > So, if we wanted to do what my patch did, we would need to come up > with some new (optional) insn attribute, say get_attr_undivisible (insn), > e.g. define that attribute on i386 to "type" attribute equal to "multi" > by default and perhaps with some exceptions if needed, and use that > attribute if HAVE_ATTR_undivisible in dwarf2cfi.c to decide whether it can > be emitted after the insn or needs to be emitted before it. > >>> But there is another problem, instruction patterns that emit multiple >>> hw instructions, code can stop in between them. So, do we want some >>> instruction attribute that would conservatively tell us which instruction >>> patterns are guaranteed to be single machine instructions? >> >> It seems to me that in that situation you'd want to add the save to *%rsp, >> and still not update to *%rbp until after the combined instruction. > > So, today I've tried instead to deal with it through REG_FRAME_RELATED_EXPR > from the backend, but that failed miserably as explained in the PR, > dwarf2cfi.c has some rules (Rule 16 to Rule 19) that are specific to the > dynamic stack realignment using drap register that only the i386 backend > does right now, and by using REG_FRAME_RELATED_EXPR or REG_CFA* notes we > can't emulate those rules. The following patch instead does the deferring > of the hard frame pointer save rule in dwarf2cfi.c Rule 18 handling and > emits it on the (set hfp sp) assignment that must appear shortly after it > and adds assertion that it is the case. > > The difference before/after the patch on the assembly is: > --- pr99334.s~ 2021-03-26 15:42:40.881749380 +0100 > +++ pr99334.s 2021-03-26 17:38:05.729161910 +0100 > @@ -11,8 +11,8 @@ _func_with_dwarf_issue_: > andq $-16, %rsp > pushq -8(%r10) > pushq %rbp > - .cfi_escape 0x10,0x6,0x2,0x76,0 > movq %rsp, %rbp > + .cfi_escape 0x10,0x6,0x2,0x76,0 > pushq %r15 > pushq %r14 > pushq %r13 > i.e. does just what we IMHO need, after pushq %rbp %rbp > still contains parent's frame value and so the save rule doesn't > need to be overridden there, ditto at the start of the next insn > before the side-effect took effect, and we override it only after > it when %rbp already has the right value. > > If some other target adds dynamic stack realignment in the future and > the offset 0 case wouldn't be true there, the code can be adjusted so that > it works on all the drap architectures, I'm pretty sure the code would > need other adjustments too. > > For the rule 18 and for the (set hfp sp) after it we already have asserts > for the drap cases that check whether the code looks the way i?86/x86_64 > emit it currently. > > Is this ok for trunk if it passes bootstrap/regtest on x86_64-linux and > i686-linux (I've verified no other target has the drap stuff right now, > nvptx does nvptx_get_drap_rtx but no other realignment stack (unclear why?), > but nvptx is a non-RA target so dwarf2cfi doesn't apply to it)? I looked into the issue a bit. So the problem is that after push %rbp, the queued reg save is trying to say that %rbp is now saved at CFA offset 0, which is correct. Except that then we hit > /* When stack is aligned, store REG using DW_CFA_expression with FP. */ > if (fde && fde->stack_realign) > { > cfi->dw_cfi_opc = DW_CFA_expression; > cfi->dw_cfi_oprnd1.dw_cfi_reg_num = reg; > cfi->dw_cfi_oprnd2.dw_cfi_loc > = build_cfa_aligned_loc (&cur_row->cfa, offset, > fde->stack_realignment); > } Which turns it into saying %rbp is saved at an offset from %rbp, which is not yet true; we can't safely do this transformation until the FP is actually set up. This code goes back to r138335, which introduced the DRAP handling. Your patch means that we don't emit the save until after the FP is set up, which works since this instruction is atomic: before the instruction, %rbp is in %rbp; after the instruction, %rbp is at *%rbp. Since this is specifically a problem with setting the frame pointer, your patch seems like a reasonable solution, assuming no other reg saves are ever emitted before this one. The patch is OK. > 2021-03-26 Jakub Jelinek > > PR debug/99334 > * dwarf2out.h (struct dw_fde_node): Add rule18 member. > * dwarf2cfi.c (dwarf2out_frame_debug_expr): When handling (set hfp sp) > assignment with drap_reg active, queue reg save for hfp with offset 0 > and flush queued reg saves. When handling a push with rule18, > defer queueing reg save for hfp and just assert the offset is 0. > (scan_trace): Assert that fde->rule18 is false. > > --- gcc/dwarf2out.h.jj 2021-01-04 10:25:38.718235126 +0100 > +++ gcc/dwarf2out.h 2021-03-26 17:12:30.599007479 +0100 > @@ -108,6 +108,12 @@ struct GTY(()) dw_fde_node { > /* True iff dw_fde_second_begin label is in text_section or > cold_text_section. */ > unsigned second_in_std_section : 1; > + /* True if Rule 18 described in dwarf2cfi.c is in action, i.e. for dynamic > + stack realignment in between pushing of hard frame pointer to stack > + and setting hard frame pointer to stack pointer. The register save for > + hard frame pointer register should be emitted only on the latter > + instruction. */ > + unsigned rule18 : 1; > }; > > > --- gcc/dwarf2cfi.c.jj 2021-03-23 19:42:05.000000000 +0100 > +++ gcc/dwarf2cfi.c 2021-03-26 17:31:58.133165709 +0100 > @@ -1695,9 +1695,19 @@ dwarf2out_frame_debug_expr (rtx expr) > if (fde > && fde->stack_realign > && REGNO (src) == STACK_POINTER_REGNUM) > - gcc_assert (REGNO (dest) == HARD_FRAME_POINTER_REGNUM > - && fde->drap_reg != INVALID_REGNUM > - && cur_cfa->reg != dwf_regno (src)); > + { > + gcc_assert (REGNO (dest) == HARD_FRAME_POINTER_REGNUM > + && fde->drap_reg != INVALID_REGNUM > + && cur_cfa->reg != dwf_regno (src) > + && fde->rule18); > + fde->rule18 = 0; > + /* The save of hard frame pointer has been deferred > + until this point when Rule 18 applied. Emit it now. */ > + queue_reg_save (dest, NULL_RTX, 0); > + /* And as the instruction modifies the hard frame pointer, > + flush the queue as well. */ > + dwarf2out_flush_queued_reg_saves (); > + } > else > queue_reg_save (src, dest, 0); > } > @@ -1907,6 +1917,7 @@ dwarf2out_frame_debug_expr (rtx expr) > { > gcc_assert (cur_cfa->reg != dw_frame_pointer_regnum); > cur_trace->cfa_store.offset = 0; > + fde->rule18 = 1; > } > > if (cur_cfa->reg == dw_stack_pointer_regnum) > @@ -2041,7 +2052,17 @@ dwarf2out_frame_debug_expr (rtx expr) > span = NULL; > > if (!span) > - queue_reg_save (src, NULL_RTX, offset); > + { > + if (fde->rule18) > + /* Just verify the hard frame pointer save when doing dynamic > + realignment uses expected offset. The actual queue_reg_save > + needs to be deferred until the instruction that sets > + hard frame pointer to stack pointer, see PR99334 for > + details. */ > + gcc_assert (known_eq (offset, 0)); > + else > + queue_reg_save (src, NULL_RTX, offset); > + } > else > { > /* We have a PARALLEL describing where the contents of SRC live. > @@ -2732,6 +2753,7 @@ scan_trace (dw_trace_info *trace, bool e > create_trace_edges (control); > } > > + gcc_assert (!cfun->fde || !cfun->fde->rule18); > add_cfi_insn = NULL; > cur_row = NULL; > cur_trace = NULL; > > > Jakub >