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.
