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.
>
