Message ID | 20220427092749.1259857-1-iii@linux.ibm.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> 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 E0B3C3857805 for <patchwork@sourceware.org>; Wed, 27 Apr 2022 09:29:09 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E0B3C3857805 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1651051749; bh=tWd7BH7UVz2vftuzqrpuJXTSezLHQ6lx7UxnzPFXsB8=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=GORo32Qep3swgPcEhMy03O9kKTP45VlmwuMG2njK9F616bF+qpKGULdE0i1H2wDrS 2GsFGk+dW8vORjmdzbJhwrJIGejb2C60PCtmYwUWRatTPY1fYg3LNYAZZ2Qd86sJgW eB27hE4sXeX7cL8rSr6VWjIQnKYhWuv4b+OH2wQU= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id EA7DB3857830; Wed, 27 Apr 2022 09:27:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EA7DB3857830 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 23R6XpnQ027446; Wed, 27 Apr 2022 09:27:59 GMT Received: from ppma03fra.de.ibm.com (6b.4a.5195.ip4.static.sl-reverse.com [149.81.74.107]) by mx0b-001b2d01.pphosted.com (PPS) with ESMTPS id 3fprsg2gbq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 27 Apr 2022 09:27:59 +0000 Received: from pps.filterd (ppma03fra.de.ibm.com [127.0.0.1]) by ppma03fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 23R98rwu020349; Wed, 27 Apr 2022 09:27:57 GMT Received: from b06avi18626390.portsmouth.uk.ibm.com (b06avi18626390.portsmouth.uk.ibm.com [9.149.26.192]) by ppma03fra.de.ibm.com with ESMTP id 3fm938vga7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 27 Apr 2022 09:27:57 +0000 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06avi18626390.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 23R9EoLN54329632 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 27 Apr 2022 09:14:50 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D4CDD11C058; Wed, 27 Apr 2022 09:27:53 +0000 (GMT) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8351011C04C; Wed, 27 Apr 2022 09:27:53 +0000 (GMT) Received: from heavy.ibmuc.com (unknown [9.171.50.79]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 27 Apr 2022 09:27:53 +0000 (GMT) To: gcc-patches@gcc.gnu.org Subject: [PATCH] Honor COMDAT for mergeable constant sections Date: Wed, 27 Apr 2022 11:27:49 +0200 Message-Id: <20220427092749.1259857-1-iii@linux.ibm.com> X-Mailer: git-send-email 2.35.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: vfP6SL2dLjCk8g08FKmXfWG3Q2Be1y9o X-Proofpoint-ORIG-GUID: vfP6SL2dLjCk8g08FKmXfWG3Q2Be1y9o X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.858,Hydra:6.0.486,FMLib:17.11.64.514 definitions=2022-04-27_03,2022-04-26_02,2022-02-23_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 impostorscore=0 adultscore=0 priorityscore=1501 malwarescore=0 mlxlogscore=807 spamscore=0 phishscore=0 lowpriorityscore=0 clxscore=1011 bulkscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000 definitions=main-2204270060 X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Ilya Leoshkevich via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Ilya Leoshkevich <iii@linux.ibm.com> Cc: Andreas Krebbel <krebbel@gcc.gnu.org> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Honor COMDAT for mergeable constant sections
|
|
Commit Message
Ilya Leoshkevich
April 27, 2022, 9:27 a.m. UTC
Bootstrapped and regtested on x86_64-redhat-linux and s390x-redhat-linux. Ok for master (or GCC 13 in case this doesn't fit stage4 criteria)? Building C++ template-heavy code with ASan sometimes leads to bogus "defined in discarded section" linker errors. The reason is that .rodata.FUNC.cstN sections are not placed into COMDAT group sections FUNC. This is important, because ASan puts references to .LASANPC labels into these sections. Discarding the respective .text.FUNC section causes the linker error. Fix by adding SECTION_LINKONCE to .rodata.FUNC.cstN sections in mergeable_constant_section () if the current function has an associated COMDAT group. This is similar to what switch_to_exception_section () is currently doing with .gcc_except_table.FUNC sections. gcc/ChangeLog: * varasm.cc (mergeable_constant_section): Honor COMDAT. gcc/testsuite/ChangeLog: * g++.dg/asan/comdat.C: New test. --- gcc/testsuite/g++.dg/asan/comdat.C | 35 ++++++++++++++++++++++++++++++ gcc/varasm.cc | 6 ++++- 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/asan/comdat.C
Comments
On Wed, Apr 27, 2022 at 11:27:49AM +0200, Ilya Leoshkevich via Gcc-patches wrote: > Bootstrapped and regtested on x86_64-redhat-linux and > s390x-redhat-linux. Ok for master (or GCC 13 in case this doesn't fit > stage4 criteria)? I'd prefer to defer this to GCC 13 at this point. Furthermore, does the linker then actually merge the constants with the same constants from other mergeable linkonce sections or other mergeable sections? I'm afraid it would only merge constants within each comdat group and not across the whole ELF object. Jakub
On Wed, 2022-04-27 at 11:33 +0200, Jakub Jelinek wrote: > On Wed, Apr 27, 2022 at 11:27:49AM +0200, Ilya Leoshkevich via Gcc- > patches wrote: > > Bootstrapped and regtested on x86_64-redhat-linux and > > s390x-redhat-linux. Ok for master (or GCC 13 in case this doesn't > > fit > > stage4 criteria)? > > I'd prefer to defer this to GCC 13 at this point. > Furthermore, does the linker then actually merge the constants with > the same constants from other mergeable linkonce sections or other > mergeable sections? I'm afraid it would only merge constants within > each comdat group and not across the whole ELF object. > > Jakub > I experimented with this a little, and actually having a reloc prevents merging altogether (the check happens in _bfd_add_merge_section). I get a .LASANPC reloc there in the first place because of https://patchwork.ozlabs.org/project/gcc/patch/20190702085154.26981-1-iii@linux.ibm.com/ but of course it may happen for other reasons as well.
On Wed, 2022-04-27 at 11:59 +0200, Ilya Leoshkevich via Gcc-patches wrote: > On Wed, 2022-04-27 at 11:33 +0200, Jakub Jelinek wrote: > > On Wed, Apr 27, 2022 at 11:27:49AM +0200, Ilya Leoshkevich via Gcc- > > patches wrote: > > > Bootstrapped and regtested on x86_64-redhat-linux and > > > s390x-redhat-linux. Ok for master (or GCC 13 in case this > > > doesn't > > > fit > > > stage4 criteria)? > > > > I'd prefer to defer this to GCC 13 at this point. > > Furthermore, does the linker then actually merge the constants with > > the same constants from other mergeable linkonce sections or other > > mergeable sections? I'm afraid it would only merge constants > > within > > each comdat group and not across the whole ELF object. > > > > Jakub > > > > I experimented with this a little, and actually having a reloc > prevents > merging altogether (the check happens in _bfd_add_merge_section). > > I get a .LASANPC reloc there in the first place because of > https://patchwork.ozlabs.org/project/gcc/patch/20190702085154.26981-1-iii@linux.ibm.com/ > but of course it may happen for other reasons as well. I just realized I forgot to mention the "normal" case. There, "aMG" seems to works fine with the whole ELF: $ cat 1.s .globl _start _start: ret .section .rodata.xxx,"aMG",@progbits,8,.xxx,comdat .quad 42 $ cat 2.s .section .rodata.yyy,"aMG",@progbits,8,.yyy,comdat .quad 42 .quad 43 .section .rodata.xxx,"aMG",@progbits,8,.xxx,comdat .quad 42 $ gcc -nostartfiles -fPIE 1.s 2.s $ objdump -D a.out 0000000000002000 <.rodata>: 2000: 2a 00 sub (%rax),%al 2002: 00 00 add %al,(%rax) 2004: 00 00 add %al,(%rax) 2006: 00 00 add %al,(%rax) 2008: 2b 00 sub (%rax),%eax 200a: 00 00 add %al,(%rax) 200c: 00 00 add %al,(%rax) ...
On Wed, Apr 27, 2022 at 11:59:49AM +0200, Ilya Leoshkevich wrote: > I get a .LASANPC reloc there in the first place because of > https://patchwork.ozlabs.org/project/gcc/patch/20190702085154.26981-1-iii@linux.ibm.com/ > but of course it may happen for other reasons as well. In that case I don't see any benefit to put that into a mergeable section. Why does that happen? Jakub
On Wed, Apr 27, 2022 at 02:23:00PM +0200, Jakub Jelinek wrote: > On Wed, Apr 27, 2022 at 11:59:49AM +0200, Ilya Leoshkevich wrote: > > I get a .LASANPC reloc there in the first place because of > > https://patchwork.ozlabs.org/project/gcc/patch/20190702085154.26981-1-iii@linux.ibm.com/ > > but of course it may happen for other reasons as well. > > In that case I don't see any benefit to put that into a mergeable section. > Why does that happen? Because, when a mergeable section doesn't contain any relocations, I don't see any point in making it comdat. Because mergeable sections themselves are garbage collected, if some constant isn't referenced at all, it isn't emitted, or if referenced, multiple copies of the constant are merged (or for mergeable strings even string tail merging is performed). Jakub
On Wed, 2022-04-27 at 14:46 +0200, Jakub Jelinek wrote: > On Wed, Apr 27, 2022 at 02:23:00PM +0200, Jakub Jelinek wrote: > > On Wed, Apr 27, 2022 at 11:59:49AM +0200, Ilya Leoshkevich wrote: > > > I get a .LASANPC reloc there in the first place because of > > > https://patchwork.ozlabs.org/project/gcc/patch/20190702085154.26981-1-iii@linux.ibm.com/ > > > but of course it may happen for other reasons as well. > > > > In that case I don't see any benefit to put that into a mergeable > > section. > > Why does that happen? > > Because, when a mergeable section doesn't contain any relocations, I > don't > see any point in making it comdat. Because mergeable sections > themselves > are garbage collected, if some constant isn't referenced at all, it > isn't > emitted, or if referenced, multiple copies of the constant are merged > (or > for mergeable strings even string tail merging is performed). > > Jakub > This is determined by default_elf_select_rtx_section (). If we don't want to mix non-reloc and reloc constants, we need to define a special section there. It seems to me, however, that this all would be made purely for the sake of .LASANPC, which is quite special: it's local, but at the same time it might need to be comdat. I don't think anything like this can appear from compiling C/C++ code. Therefore I wonder if we could just drop it altogether like this? @@ -1928,22 +1919,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, ... - emit_move_insn (mem, expand_normal (build_fold_addr_expr (decl))); + emit_move_insn (mem, expand_normal (build_fold_addr_expr (current_function_decl))); ... That's what LLVM is already doing. This will also solve the alignment problem I referred to earlier.
On Thu, Apr 28, 2022 at 01:03:26PM +0200, Ilya Leoshkevich wrote: > This is determined by default_elf_select_rtx_section (). If we don't > want to mix non-reloc and reloc constants, we need to define a special > section there. > > It seems to me, however, that this all would be made purely for the > sake of .LASANPC, which is quite special: it's local, but at the same > time it might need to be comdat. I don't think anything like this can > appear from compiling C/C++ code. > > Therefore I wonder if we could just drop it altogether like this? > > @@ -1928,22 +1919,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, > unsigned int alignb, > ... > - emit_move_insn (mem, expand_normal (build_fold_addr_expr (decl))); > + emit_move_insn (mem, expand_normal (build_fold_addr_expr > (current_function_decl))); > ... > > That's what LLVM is already doing. This will also solve the alignment > problem I referred to earlier. LLVM is doing a wrong thing here. The global symbol can be overridden by a symbol in another shared library, that is definitely not what we want, because the ASAN record is for the particular implementation, not the other one which could be quite different. I think the right fix would be: --- gcc/varasm.cc.jj 2022-03-07 15:00:17.255592497 +0100 +++ gcc/varasm.cc 2022-04-28 13:22:44.463147066 +0200 @@ -7326,6 +7326,9 @@ default_elf_select_rtx_section (machine_ return get_named_section (NULL, ".data.rel.ro", 3); } + if (reloc) + return readonly_data_section; + return mergeable_constant_section (mode, align, 0); } which matches what we do in categorize_decl_for_section: else if (reloc & targetm.asm_out.reloc_rw_mask ()) ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO; else if (reloc || flag_merge_constants < 2 ... /* C and C++ don't allow different variables to share the same location. -fmerge-all-constants allows even that (at the expense of not conforming). */ ret = SECCAT_RODATA; else if (DECL_INITIAL (decl) && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST) ret = SECCAT_RODATA_MERGE_STR_INIT; else ret = SECCAT_RODATA_MERGE_CONST; i.e. if reloc is true, it goes into .data.rel.ro* for -fpic and .rodata for non-pic, and mergeable sections are only used if there are no relocations. Anyway, I'd feel much safer to change it only in GCC 13, at least initially. Or are some linkers (say lld or mold, fod ld.bfd I'm pretty sure it doesn't, for gold no idea but unlikely) able to merge even constants with relocations against them? Jakub
On Thu, 2022-04-28 at 13:27 +0200, Jakub Jelinek wrote: > On Thu, Apr 28, 2022 at 01:03:26PM +0200, Ilya Leoshkevich wrote: > > This is determined by default_elf_select_rtx_section (). If we > > don't > > want to mix non-reloc and reloc constants, we need to define a > > special > > section there. > > > > It seems to me, however, that this all would be made purely for the > > sake of .LASANPC, which is quite special: it's local, but at the > > same > > time it might need to be comdat. I don't think anything like this > > can > > appear from compiling C/C++ code. > > > > Therefore I wonder if we could just drop it altogether like this? > > > > @@ -1928,22 +1919,7 @@ asan_emit_stack_protection (rtx base, rtx > > pbase, > > unsigned int alignb, > > ... > > - emit_move_insn (mem, expand_normal (build_fold_addr_expr > > (decl))); > > + emit_move_insn (mem, expand_normal (build_fold_addr_expr > > (current_function_decl))); > > ... > > > > That's what LLVM is already doing. This will also solve the > > alignment > > problem I referred to earlier. > > LLVM is doing a wrong thing here. The global symbol can be > overridden by > a symbol in another shared library, that is definitely not what we > want, > because the ASAN record is for the particular implementation, not the > other > one which could be quite different. I see; this must be relevant when the overriding library calls the original one through dlsym (RTLD_NEXT). > I think the right fix would be: > --- gcc/varasm.cc.jj 2022-03-07 15:00:17.255592497 +0100 > +++ gcc/varasm.cc 2022-04-28 13:22:44.463147066 +0200 > @@ -7326,6 +7326,9 @@ default_elf_select_rtx_section (machine_ > return get_named_section (NULL, ".data.rel.ro", 3); > } > > + if (reloc) > + return readonly_data_section; > + > return mergeable_constant_section (mode, align, 0); > } > > which matches what we do in categorize_decl_for_section: > else if (reloc & targetm.asm_out.reloc_rw_mask ()) > ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : > SECCAT_DATA_REL_RO; > else if (reloc || flag_merge_constants < 2 > ... > /* C and C++ don't allow different variables to share the > same > location. -fmerge-all-constants allows even that (at the > expense of not conforming). */ > ret = SECCAT_RODATA; > else if (DECL_INITIAL (decl) > && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST) > ret = SECCAT_RODATA_MERGE_STR_INIT; > else > ret = SECCAT_RODATA_MERGE_CONST; > i.e. if reloc is true, it goes into .data.rel.ro* for -fpic and > .rodata > for non-pic, and mergeable sections are only used if there are no > relocations. This doesn't resolve the problem, unfortunately, because references to discarded comdat symbols are still kept in .rodata: `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_' referenced in section `.rodata' of ../lib/libgtest.a(gtest-all.cc.o): defined in discarded section `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_[_ZN7testing15Assert ionResultlsIPKcEERS0_RKT_]' of ../lib/libgtest.a(gtest-all.cc.o) (That's from building zlib-ng with ASan and your patch on s390). So I was rather thinking about adding a reloc parameter to mergeable_constant_section () and slightly changing the section name when it's nonzero, e.g. from .cst to .cstrel. > Anyway, I'd feel much safer to change it only in GCC 13, at least > initially. That's fine with me. > Or are some linkers (say lld or mold, fod ld.bfd I'm pretty sure it > doesn't, > for gold no idea but unlikely) able to merge even constants with > relocations against them? I'm not sure, but putting constants with relocations into a separate mergeable section shouldn't hurt too much. And if such a linker is implemented some day, there would be no need to tweak gcc.
On Thu, 2022-04-28 at 14:05 +0200, Ilya Leoshkevich wrote: > On Thu, 2022-04-28 at 13:27 +0200, Jakub Jelinek wrote: > > On Thu, Apr 28, 2022 at 01:03:26PM +0200, Ilya Leoshkevich wrote: > > > This is determined by default_elf_select_rtx_section (). If we > > > don't > > > want to mix non-reloc and reloc constants, we need to define a > > > special > > > section there. > > > > > > It seems to me, however, that this all would be made purely for > > > the > > > sake of .LASANPC, which is quite special: it's local, but at the > > > same > > > time it might need to be comdat. I don't think anything like > > > this > > > can > > > appear from compiling C/C++ code. > > > > > > Therefore I wonder if we could just drop it altogether like this? > > > > > > @@ -1928,22 +1919,7 @@ asan_emit_stack_protection (rtx base, rtx > > > pbase, > > > unsigned int alignb, > > > ... > > > - emit_move_insn (mem, expand_normal (build_fold_addr_expr > > > (decl))); > > > + emit_move_insn (mem, expand_normal (build_fold_addr_expr > > > (current_function_decl))); > > > ... > > > > > > That's what LLVM is already doing. This will also solve the > > > alignment > > > problem I referred to earlier. > > > > LLVM is doing a wrong thing here. The global symbol can be > > overridden by > > a symbol in another shared library, that is definitely not what we > > want, > > because the ASAN record is for the particular implementation, not > > the > > other > > one which could be quite different. > > I see; this must be relevant when the overriding library calls > the original one through dlsym (RTLD_NEXT). > > > I think the right fix would be: > > --- gcc/varasm.cc.jj 2022-03-07 15:00:17.255592497 +0100 > > +++ gcc/varasm.cc 2022-04-28 13:22:44.463147066 +0200 > > @@ -7326,6 +7326,9 @@ default_elf_select_rtx_section (machine_ > > return get_named_section (NULL, ".data.rel.ro", 3); > > } > > > > + if (reloc) > > + return readonly_data_section; > > + > > return mergeable_constant_section (mode, align, 0); > > } > > > > which matches what we do in categorize_decl_for_section: > > else if (reloc & targetm.asm_out.reloc_rw_mask ()) > > ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : > > SECCAT_DATA_REL_RO; > > else if (reloc || flag_merge_constants < 2 > > ... > > /* C and C++ don't allow different variables to share the > > same > > location. -fmerge-all-constants allows even that (at > > the > > expense of not conforming). */ > > ret = SECCAT_RODATA; > > else if (DECL_INITIAL (decl) > > && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST) > > ret = SECCAT_RODATA_MERGE_STR_INIT; > > else > > ret = SECCAT_RODATA_MERGE_CONST; > > i.e. if reloc is true, it goes into .data.rel.ro* for -fpic and > > .rodata > > for non-pic, and mergeable sections are only used if there are no > > relocations. > > This doesn't resolve the problem, unfortunately, because > references to discarded comdat symbols are still kept in .rodata: > > `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_' referenced in > section `.rodata' of ../lib/libgtest.a(gtest-all.cc.o): defined in > discarded section > `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_[_ZN7testing15Asse > rt > ionResultlsIPKcEERS0_RKT_]' of ../lib/libgtest.a(gtest-all.cc.o) > > (That's from building zlib-ng with ASan and your patch on s390). > > So I was rather thinking about adding a reloc parameter to > mergeable_constant_section () and slightly changing the section > name when it's nonzero, e.g. from .cst to .cstrel. After some experimenting, I don't think that what I propose here is a good solution anymore, since it won't work with -fno-merge-constants. What do you think about something like this? --- a/gcc/varasm.cc +++ b/gcc/varasm.cc @@ -7326,6 +7326,10 @@ default_elf_select_rtx_section (machine_mode mode, rtx x, return get_named_section (NULL, ".data.rel.ro", 3); } + if (reloc) + return targetm.asm_out.function_rodata_section (current_function_decl, + false); + return mergeable_constant_section (mode, align, 0); } This would put constants with relocations into .rodata.<FUNC>. default_function_rodata_section () already ensures that these sections are in the right comdat group. >
On Fri, Apr 29, 2022 at 01:52:49PM +0200, Ilya Leoshkevich wrote: > > This doesn't resolve the problem, unfortunately, because > > references to discarded comdat symbols are still kept in .rodata: > > > > `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_' referenced in > > section `.rodata' of ../lib/libgtest.a(gtest-all.cc.o): defined in > > discarded section > > `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_[_ZN7testing15Asse > > rt > > ionResultlsIPKcEERS0_RKT_]' of ../lib/libgtest.a(gtest-all.cc.o) > > > > (That's from building zlib-ng with ASan and your patch on s390). > > > > So I was rather thinking about adding a reloc parameter to > > mergeable_constant_section () and slightly changing the section > > name when it's nonzero, e.g. from .cst to .cstrel. > > After some experimenting, I don't think that what I propose here > is a good solution anymore, since it won't work with > -fno-merge-constants. > > What do you think about something like this? > > --- a/gcc/varasm.cc > +++ b/gcc/varasm.cc > @@ -7326,6 +7326,10 @@ default_elf_select_rtx_section (machine_mode > mode, rtx x, > return get_named_section (NULL, ".data.rel.ro", 3); > } > > + if (reloc) > + return targetm.asm_out.function_rodata_section > (current_function_decl, > + false); > + > return mergeable_constant_section (mode, align, 0); > } > > This would put constants with relocations into .rodata.<FUNC>. > default_function_rodata_section () already ensures that these sections > are in the right comdat group. We don't really know if the emitted constant is purely for the current function, or also other functions (say emitted in as constant pool constant where constant pool constants are shared across the whole TU). For the former, putting it into current function's comdat is fine, for the latter certainly isn't. Jakub
On Fri, 2022-04-29 at 13:56 +0200, Jakub Jelinek wrote: > On Fri, Apr 29, 2022 at 01:52:49PM +0200, Ilya Leoshkevich wrote: > > > This doesn't resolve the problem, unfortunately, because > > > references to discarded comdat symbols are still kept in .rodata: > > > > > > `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_' referenced > > > in > > > section `.rodata' of ../lib/libgtest.a(gtest-all.cc.o): defined > > > in > > > discarded section > > > `.text._ZN7testing15AssertionResultlsIPKcEERS0_RKT_[_ZN7testing15 > > > Asse > > > rt > > > ionResultlsIPKcEERS0_RKT_]' of ../lib/libgtest.a(gtest-all.cc.o) > > > > > > (That's from building zlib-ng with ASan and your patch on s390). > > > > > > So I was rather thinking about adding a reloc parameter to > > > mergeable_constant_section () and slightly changing the section > > > name when it's nonzero, e.g. from .cst to .cstrel. > > > > After some experimenting, I don't think that what I propose here > > is a good solution anymore, since it won't work with > > -fno-merge-constants. > > > > What do you think about something like this? > > > > --- a/gcc/varasm.cc > > +++ b/gcc/varasm.cc > > @@ -7326,6 +7326,10 @@ default_elf_select_rtx_section (machine_mode > > mode, rtx x, > > return get_named_section (NULL, ".data.rel.ro", 3); > > } > > > > + if (reloc) > > + return targetm.asm_out.function_rodata_section > > (current_function_decl, > > + false); > > + > > return mergeable_constant_section (mode, align, 0); > > } > > > > This would put constants with relocations into .rodata.<FUNC>. > > default_function_rodata_section () already ensures that these > > sections > > are in the right comdat group. > > We don't really know if the emitted constant is purely for the > current > function, or also other functions (say emitted in as constant pool > constant > where constant pool constants are shared across the whole TU). > For the former, putting it into current function's comdat is fine, > for the > latter certainly isn't. mergeable_constant_section (), that the existing code calls in the same context, already relies on this being known and calls function_rodata_section () with exactly the same arguments. If !current_function_decl && !relocatable, we get readonly_data_section. Of course, mergeable_constant_section () does not handle comdat currently, so this point might be moot. However, looking at the callers of output_constant_pool_contents (), it seems that !current_function_decl happens in and only in the shared_constant_pool case, so it looks as if we know whether the constant is tied to a single function or not.
diff --git a/gcc/testsuite/g++.dg/asan/comdat.C b/gcc/testsuite/g++.dg/asan/comdat.C new file mode 100644 index 00000000000..cd4f3f830a8 --- /dev/null +++ b/gcc/testsuite/g++.dg/asan/comdat.C @@ -0,0 +1,35 @@ +/* Check that we don't emit non-COMDAT rodata. */ + +/* { dg-do compile } */ +/* { dg-final { scan-assembler-not {\.section\t\.rodata\._ZN1hlsIPKcEERS_RKT_\.cst[48],"[^"]*",@progbits,[48]\n} } } */ + +const char *a; + +class b +{ +public: + b (); +}; + +class h +{ +public: + template <typename c> + h & + operator<< (const c &) + { + d (b ()); + return *this; + } + + void d (b); +}; + +h e (); + +h +g () +{ + e () << a << a << a; + throw; +} diff --git a/gcc/varasm.cc b/gcc/varasm.cc index c41f17d64f7..f2614f0ee39 100644 --- a/gcc/varasm.cc +++ b/gcc/varasm.cc @@ -938,7 +938,11 @@ mergeable_constant_section (machine_mode mode ATTRIBUTE_UNUSED, sprintf (name, "%s.cst%d", prefix, (int) (align / 8)); flags |= (align / 8) | SECTION_MERGE; - return get_section (name, flags, NULL); + if (current_function_decl + && DECL_COMDAT_GROUP (current_function_decl) + && HAVE_COMDAT_GROUP) + flags |= SECTION_LINKONCE; + return get_section (name, flags, current_function_decl); } return readonly_data_section; }