From patchwork Mon Oct 4 09:31:26 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 45780 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A20DB3858039 for ; Mon, 4 Oct 2021 09:32:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A20DB3858039 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1633339927; bh=OWfEFdWr86SC5gk/skDDk7APFdcgn2+RUj7P1cwFqQI=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=xSbWvWKGRyuVIa5MaBUNG6zP5But36QSLqCRP2hYSY+UU13oNS6Uawje/qf/Th19U W/1ojV0Ri8R5bICcoKauMPQ6QDT0MUZxTkIPXfdb2BClozNYbcFFHAFJIGjQIuAEkw yH2v2V9imoQ9ZkF+d9D9sMBtGr49qHC40SQmx3BE= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 913063858408 for ; Mon, 4 Oct 2021 09:31:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 913063858408 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-516-qqXav0GrN36cla4yuzzx4w-1; Mon, 04 Oct 2021 05:31:33 -0400 X-MC-Unique: qqXav0GrN36cla4yuzzx4w-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2AA3DDF8A3; Mon, 4 Oct 2021 09:31:32 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.193.109]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7257960C17; Mon, 4 Oct 2021 09:31:31 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 1949VSU9392274 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Mon, 4 Oct 2021 11:31:28 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 1949VQqI392273; Mon, 4 Oct 2021 11:31:26 +0200 Date: Mon, 4 Oct 2021 11:31:26 +0200 To: Richard Biener , Alexandre Oliva , Jeff Law Subject: [PATCH] var-tracking: Fix a wrong-debug issue caused by my r10-7665 var-tracking change [PR102441] Message-ID: <20211004093126.GK304296@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jakub Jelinek via Gcc-patches From: Jakub Jelinek Reply-To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Hi! Since my r10-7665-g33c45e51b4914008064d9b77f2c1fc0eea1ad060 change, we get wrong-debug on e.g. the following testcase at -O2 -g on x86_64-linux for the x parameter: void bar (int *r); int foo (int x) { int r = 0; bar (&r); return r; } At the start of function, we have subq $24, %rsp leaq 12(%rsp), %rdi instructions. The x parameter is passed in %rdi, but isn't used in the function and so the leaq instruction overwrites %rdi without remembering %rdi anywhere. Before the r10-7665 change (which was trying to fix a large (3% for 32-bit, 1% for 64-bit x86-64) debug info/loc growth introduced with r10-7515), the leaq insn above resulted in a MO_VAL_SET micro-operation that said that the value of sp + 12, a cselib_sp_derived_value_p, is stored into the %rdi register. The r10-7665 change added a change to add_stores that added no micro-operation for the leaq store, with the rationale that the sp based values can be and will be always computable some other more compact and primarily more stable way (cfa based expression like DW_OP_fbreg, that is the same in the whole function). That is true. But by throwing the micro-operation on the floor, we miss another important part of the MO_VAL_SET, in particular that the destination of the store, %rdi in this case, now has a different value from what it had before, so the vt_* dataflow code thinks that even after the leaq instruction %rdi still holds the x argument value (and changes it to DW_OP_entry_value (%rdi) only in the middle of the call to bar). Previously and with the patches below, the location for x changes already at the end of leaq instruction to DW_OP_entry_value (%rdi). My first attempt to fix this was instead of dropping the MO_VAL_SET add a MO_CLOBBER operation: so don't track that the value lives in the loc destination, but track that the previous value doesn't live there anymore. That failed bootstrap miserably, the vt_* code isn't prepared to see MO_CLOBBER of a MEM that isn't tracked (e.g. has MEM_EXPR on it that the var-tracking code wants to track, i.e. track_p in add_stores). On the other side, thinking about it more, in the most common case where a cselib_sp_derived_value_p value is stored into the sp register (and which is the reason why PR94495 testcase got larger), dropping the micro-operation on the floor is the right thing, because we have that cselib_sp_derived_value_p tracking, any reads from the sp hard register will be treated as cselib_sp_derived_value_p. Then I've tried 3 different patches, patch{1,2,3} attached below and all of these passed bootstrap/regtest on x86_64-linux and i686-linux. Additionally, I've gathered statistics from cc1plus by always reverting the var-tracking.c change after finished bootstrap/regtest and rebuilding the stage3 var-tracking.o and cc1plus, such that it would be comparable. dwlocstat and .debug_{info,loclists} section sizes detailed below. patch3 uses MO_VAL_SET (i.e. essentially reversion of the r10-7665 change) when destination is not a REG_P and !track_p, otherwise if destination is sp drops the micro-operation on the floor (i.e. no change), otherwise adds a MO_CLOBBER. patch1 is similar, except it checks for destination not equal to sp and !track_p, i.e. for !track_p REG_P destinations other than sp it will use MO_VAL_SET rather than MO_CLOBBER. Finally, patch2, the shortest patch, uses MO_VAL_SET whenever destination is not sp and otherwise drops the micro-operation on the floor. All the 3 patches don't affect the PR94495 testcase, all the changes there were caused by stores of sp based values into %rsp. While the patch2 (and patch1 which results in exactly the same sizes) causes the largest debug loclists/info growth from the 3, it is still quite minor (0.651% on 64-bit and 0.114% on 32-bit) compared to the 1% and 3% PR94495 was trying to solve, and I actually think it is the best thing to do. Because, if we have say int q[10]; int *p = &q[0]; or similar and we load the &q[0] sp based value into some hard register, by noting in the debug info that p lives in some hard reg for some part of the function and a user is trying to change the p var in the debugger, if we say it lives in some register or memory, there is some chance that the changing of the value could work successfully (of course, nothing is guaranteed, we don't have tracking of where each var lives at which moment for changing purposes (i.e. what register, memory or else you need to change in order to change behavior of the code)), while if we just say that p's location is DW_OP_fbreg 16 DW_OP_stack_value, that is a read-only value one can just print but not change. Now, for stores of variable values into the sp register, I don't think we have such an issue, you don't want debugger to change your stack pointer when user asks to change value of some variable whose value lives in the stack pointer, that would pretty much always result in misbehavior of the program. So, my preference from these 3 is patch2. Ok for trunk (which one)? 64-bit cc1plus ============== vanilla cov% samples cumul 0..10 1064665/37% 1064665/37% 11..20 35972/1% 1100637/38% 21..30 47969/1% 1148606/40% 31..40 45787/1% 1194393/42% 41..50 57529/2% 1251922/44% 51..60 53974/1% 1305896/46% 61..70 112055/3% 1417951/50% 71..80 79420/2% 1497371/52% 81..90 126225/4% 1623596/57% 91..100 1206682/42% 2830278/100% [34] .debug_info PROGBITS 0000000000000000 2f1c74c a44949f 00 0 0 1 [38] .debug_loclists PROGBITS 0000000000000000 ff5d046 506e947 00 0 0 1 patch1 (same as patch2) cov% samples cumul 0..10 1064685/37% 1064685/37% 11..20 36011/1% 1100696/38% 21..30 47975/1% 1148671/40% 31..40 45799/1% 1194470/42% 41..50 57566/2% 1252036/44% 51..60 54011/1% 1306047/46% 61..70 112068/3% 1418115/50% 71..80 79421/2% 1497536/52% 81..90 126171/4% 1623707/57% 91..100 1206571/42% 2830278/100% [34] .debug_info PROGBITS 0000000000000000 2f1c74c a448f27 00 0 0 1 [38] .debug_loclists PROGBITS 0000000000000000 ff608bc 52070dd 00 0 0 1 patch3 cov% samples cumul 0..10 1064698/37% 1064698/37% 11..20 36018/1% 1100716/38% 21..30 47977/1% 1148693/40% 31..40 45804/1% 1194497/42% 41..50 57562/2% 1252059/44% 51..60 54018/1% 1306077/46% 61..70 112071/3% 1418148/50% 71..80 79424/2% 1497572/52% 81..90 126172/4% 1623744/57% 91..100 1206534/42% 2830278/100% [34] .debug_info PROGBITS 0000000000000000 2f1c74c a449548 00 0 0 1 [38] .debug_loclists PROGBITS 0000000000000000 ff5df39 507acd8 00 0 0 1 So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or patch2) by 0.651% and for vanilla -> patch3 by 0.020%. 32-bit cc1plus ============== vanilla cov% samples cumul 0..10 1061892/37% 1061892/37% 11..20 34002/1% 1095894/39% 21..30 43513/1% 1139407/40% 31..40 41667/1% 1181074/42% 41..50 59144/2% 1240218/44% 51..60 47009/1% 1287227/45% 61..70 105069/3% 1392296/49% 71..80 72990/2% 1465286/52% 81..90 125988/4% 1591274/56% 91..100 1208726/43% 2800000/100% [33] .debug_info PROGBITS 00000000 351ab10 8b1c83d 00 0 0 1 [37] .debug_loclists PROGBITS 00000000 ebc816e 3fe44fd 00 0 0 1 patch1 (same as patch2) cov% samples cumul 0..10 1061999/37% 1061999/37% 11..20 34065/1% 1096064/39% 21..30 43557/1% 1139621/40% 31..40 41690/1% 1181311/42% 41..50 59191/2% 1240502/44% 51..60 47143/1% 1287645/45% 61..70 105045/3% 1392690/49% 71..80 73021/2% 1465711/52% 81..90 125885/4% 1591596/56% 91..100 1208404/43% 2800000/100% [33] .debug_info PROGBITS 00000000 351ab10 8b1c597 00 0 0 1 [37] .debug_loclists PROGBITS 00000000 ebca915 401ffad 00 0 0 1 patch3 cov% samples cumul 0..10 1062006/37% 1062006/37% 11..20 34073/1% 1096079/39% 21..30 43559/1% 1139638/40% 31..40 41693/1% 1181331/42% 41..50 59189/2% 1240520/44% 51..60 47142/1% 1287662/45% 61..70 105054/3% 1392716/49% 71..80 73027/2% 1465743/52% 81..90 125874/4% 1591617/56% 91..100 1208383/43% 2800000/100% [33] .debug_info PROGBITS 00000000 351ab10 8b1c690 00 0 0 1 [37] .debug_loclists PROGBITS 00000000 ebca40a 4020a6e 00 0 0 1 So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or patch2) by 0.114% and for vanilla -> patch3 by 0.116%. Jakub 2021-10-04 Jakub Jelinek PR debug/102441 * var-tracking.c (add_stores): For cselib_sp_derived_value_p values use MO_VAL_SET if !track_p and loc is not sp, otherwise emit a MO_CLOBBER microoperation unless loc is sp. --- gcc/var-tracking.c.jj 2021-09-27 23:34:52.290324697 +0200 +++ gcc/var-tracking.c 2021-10-01 18:51:07.539130485 +0200 @@ -6129,11 +6129,16 @@ add_stores (rtx loc, const_rtx expr, voi on which register holds that VALUE in some instruction. */ if (!frame_pointer_needed && cfa_base_rtx - && cselib_sp_derived_value_p (v)) + && cselib_sp_derived_value_p (v) + && (loc == stack_pointer_rtx || track_p)) { if (preserve) preserve_value (v); - return; + if (loc == stack_pointer_rtx) + return; + mo.type = MO_CLOBBER; + mo.u.loc = loc; + goto log_and_return; } nloc = replace_expr_with_values (oloc); 2021-10-04 Jakub Jelinek PR debug/102441 * var-tracking.c (add_stores): For cselib_sp_derived_value_p values use MO_VAL_SET if loc is not sp. --- gcc/var-tracking.c.jj 2021-09-27 23:34:52.290324697 +0200 +++ gcc/var-tracking.c 2021-10-02 21:46:54.420433439 +0200 @@ -6129,7 +6129,8 @@ add_stores (rtx loc, const_rtx expr, voi on which register holds that VALUE in some instruction. */ if (!frame_pointer_needed && cfa_base_rtx - && cselib_sp_derived_value_p (v)) + && cselib_sp_derived_value_p (v) + && loc == stack_pointer_rtx) { if (preserve) preserve_value (v); 2021-10-04 Jakub Jelinek PR debug/102441 * var-tracking.c (add_stores): For cselib_sp_derived_value_p values use MO_VAL_SET if !track_p and loc is not a REG, otherwise emit a MO_CLOBBER microoperation unless loc is sp. --- gcc/var-tracking.c.jj 2021-09-27 23:34:52.290324697 +0200 +++ gcc/var-tracking.c 2021-10-03 21:29:21.683588426 +0200 @@ -6129,11 +6129,16 @@ add_stores (rtx loc, const_rtx expr, voi on which register holds that VALUE in some instruction. */ if (!frame_pointer_needed && cfa_base_rtx - && cselib_sp_derived_value_p (v)) + && cselib_sp_derived_value_p (v) + && (REG_P (loc) || track_p)) { if (preserve) preserve_value (v); - return; + if (loc == stack_pointer_rtx) + return; + mo.type = MO_CLOBBER; + mo.u.loc = loc; + goto log_and_return; } nloc = replace_expr_with_values (oloc); --- gcc/var-tracking.c.jj 2021-05-04 21:02:24.196799586 +0200 +++ gcc/var-tracking.c 2021-09-24 19:23:16.420154828 +0200 @@ -6133,7 +6133,9 @@ add_stores (rtx loc, const_rtx expr, voi { if (preserve) preserve_value (v); - return; + mo.type = MO_CLOBBER; + mo.u.loc = loc; + goto log_and_return; } nloc = replace_expr_with_values (oloc);