[PATCH] Fix attribute((section)) for templates
Strager Neds
strager.nds@gmail.com
Fri Nov 29 23:33:00 GMT 2019
I discovered an issue with my patch. I need help resolving it.
Take the following code for example:
template<class>
struct s
{
static inline int __attribute__((section(".testsection"))) var = 1;
};
struct public_symbol {};
namespace {
struct internal_symbol {};
}
int *
f(bool which)
{
if (which)
return &s<public_symbol>::var;
else
return &s<internal_symbol>::var;
}
With my patch, compiling this code fails with the following error:
example.C:4:62: error: 's<{anonymous}::internal_symbol>::var'
causes a section type conflict with 's<public_symbol>::var'
example.C:4:62: note: 's<public_symbol>::var' was declared here
The error is reported by gcc/varasm.c (get_section) because
s<internal_symbol>::var has the following section flags:
SECTION_NAMED | SECTION_NOTYPE | SECTION_WRITE
(flags == 0x280200)
but s<public_symbol>::var has the following section flags:
SECTION_NAMED | SECTION_LINKONCE | SECTION_WRITE
(sect->common.flags == 0x200a00)
and a section can't have both of these flag at the same time. In
particular, SECTION_LINKONCE conflicts with not-SECTION_LINKONCE.
How can we solve this problem? Some ideas (none of which I like):
* Disallow this code, possibly with an improved diagnostic.
* Silently make the section SECTION_LINKONCE if there is a conflict.
* Silently make the section not-SECTION_LINKONCE if there is a conflict.
* Silently make the section not-SECTION_LINKONCE unconditionally (even
if there is no conflict).
* Make two sections with the same name, one with SECTION_LINKONCE and
one with not-SECTION_LINKONCE. This is what Clang does. Clang seems to
Do What I Mean for ELF; the .o file has one COMDAT section and another
non-COMDAT section.
* Extend attribute((section())) to allow specifying different section
names for different section flags.
Thanks in advance for your feedback!
On Fri, Nov 22, 2019 at 12:09 PM Strager Neds <strager.nds@gmail.com> wrote:
>
> Here's a revised version of the patch. This revised version is ready for review.
>
> When GCC encounters __attribute__((section("foo"))) on a function or
> variable declaration, it adds an entry in the symbol table for the
> declaration to remember its desired section. The symbol table is
> separate from the declaration's tree node.
>
> When instantiating a template, GCC copies the tree of the template
> recursively. GCC does *not* copy symbol table entries when copying
> function and variable declarations.
>
> Combined, these two details mean that section attributes on function and
> variable declarations in a template have no effect.
>
> Fix this issue by copying the section name (in the symbol table) when
> copying a tree node for template instantiation. This addresses PR
> c++/70435 and PR c++/88061.
>
> Originally, I tried copying section names in copy_node. This caused
> problems for reasons I do not understand. This patch in this email
> avoids those problems by copying section names only in the callers of
> copy_node relevant to template instantation.
>
> Known unknowns (questions for the audience):
>
> * For all targets which support the section attribute, are functions and
> variables deduplicated (comdat) when using a custom section? It seems
> to work with GNU ELF on Linux and with Mach-O on macOS (i.e. I end up
> with only one copy), but I'm unsure about other platforms. Richard
> Biener raised this concern in PR c++/88061. Is this something I should
> worry much about?
> * Do we need to check or copy implicit_section or alias? I don't know
> what these properties mean, but they look related to section_name.
>
> Note: This patch depends on the following unmerged patches (but could be
> changed to not depend on them):
>
> * Simplify testing symbol sections:
> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02062.html
> * Fix attribute((section)) with -flto:
> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02063.html
> * Refactor copying decl section names:
> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00979.html
>
> Testing:
>
> * Bootstrap on x86_64-linux-gnu with --disable-multilib
> --enable-checking=release --enable-languages=c,c++. Observe no change
> in test results (aside from the added tests).
> * Bootstrap on macOS x86_64-apple-darwin16.7.0 with --disable-multilib
> --enable-checking=release --enable-languages=c,c++. Observe no change
> in test results (aside from the added tests).
>
> 2019-11-20 Matthew Glazar <strager.nds@gmail.com>
>
> * gcc/cp/pt.c (tsubst_function_decl): Copy the section name from the
> original function.
> (tsubst_decl): Copy the section name from the original variable (if the
> variable is global).
> ---
> gcc/cp/pt.c | 5 +++
> ...section-class-template-function-template.C | 25 +++++++++++
> ...ass-template-specialized-static-variable.C | 29 +++++++++++++
> ...template-static-inline-variable-template.C | 19 ++++++++
> ...on-class-template-static-inline-variable.C | 19 ++++++++
> .../section-class-template-static-variable.C | 20 +++++++++
> ...on-function-template-static-variable-lto.C | 19 ++++++++
> ...ection-function-template-static-variable.C | 26 +++++++++++
> .../g++.dg/ext/section-function-template.C | 20 +++++++++
> .../ext/section-multi-tu-template-main.C | 43 +++++++++++++++++++
> .../ext/section-multi-tu-template-other.C | 24 +++++++++++
> .../g++.dg/ext/section-multi-tu-template.h | 33 ++++++++++++++
> .../g++.dg/ext/section-variable-template.C | 16 +++++++
> 13 files changed, 298 insertions(+)
> create mode 100644
> gcc/testsuite/g++.dg/ext/section-class-template-function-template.C
> create mode 100644
> gcc/testsuite/g++.dg/ext/section-class-template-specialized-static-variable.C
> create mode 100644
> gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable-template.C
> create mode 100644
> gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable.C
> create mode 100644
> gcc/testsuite/g++.dg/ext/section-class-template-static-variable.C
> create mode 100644
> gcc/testsuite/g++.dg/ext/section-function-template-static-variable-lto.C
> create mode 100644
> gcc/testsuite/g++.dg/ext/section-function-template-static-variable.C
> create mode 100644 gcc/testsuite/g++.dg/ext/section-function-template.C
> create mode 100644 gcc/testsuite/g++.dg/ext/section-multi-tu-template-main.C
> create mode 100644 gcc/testsuite/g++.dg/ext/section-multi-tu-template-other.C
> create mode 100644 gcc/testsuite/g++.dg/ext/section-multi-tu-template.h
> create mode 100644 gcc/testsuite/g++.dg/ext/section-variable-template.C
>
> diff --git gcc/cp/pt.c gcc/cp/pt.c
> index 8bacb3952ff..2593cf67a20 100644
> --- gcc/cp/pt.c
> +++ gcc/cp/pt.c
> @@ -42,6 +42,7 @@ along with GCC; see the file COPYING3. If not see
> #include "gimplify.h"
> #include "gcc-rich-location.h"
> #include "selftest.h"
> +#include "cgraph.h"
>
> /* The type of functions taking a tree, and some additional data, and
> returning an int. */
> @@ -13526,6 +13527,7 @@ tsubst_function_decl (tree t, tree args,
> tsubst_flags_t complain,
> if (!DECL_DELETED_FN (r))
> DECL_INITIAL (r) = NULL_TREE;
> DECL_CONTEXT (r) = ctx;
> + set_decl_section_name (r, t);
>
> /* Handle explicit(dependent-expr). */
> if (DECL_HAS_DEPENDENT_EXPLICIT_SPEC_P (t))
> @@ -14432,6 +14434,9 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
> register_local_specialization (r, t);
> }
>
> + if (VAR_P (t) && is_global_var (t))
> + set_decl_section_name (r, t);
> +
> DECL_CHAIN (r) = NULL_TREE;
>
> apply_late_template_attributes (&r, DECL_ATTRIBUTES (r),
> diff --git gcc/testsuite/g++.dg/ext/section-class-template-function-template.C
> gcc/testsuite/g++.dg/ext/section-class-template-function-template.C
> new file mode 100644
> index 00000000000..b890cda770e
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-class-template-function-template.C
> @@ -0,0 +1,25 @@
> +// attribute((section)) should affect function templates
> +// within class templates.
> +
> +// { dg-do compile }
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {callee} {^\.testsection$} } }
> +
> +template<class>
> +struct s
> +{
> + template<class>
> + static __attribute__((section(".testsection"))) void
> + callee()
> + {
> + }
> +};
> +
> +void
> +f(bool which)
> +{
> + if (which)
> + s<int>::callee<int>();
> + else
> + s<float>::callee<float>();
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-class-template-specialized-static-variable.C
> gcc/testsuite/g++.dg/ext/section-class-template-specialized-static-variable.C
> new file mode 100644
> index 00000000000..d518da858ab
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-class-template-specialized-static-variable.C
> @@ -0,0 +1,29 @@
> +// PR c++/70435
> +// attribute((section)) should affect specialized static
> +// variables in class templates.
> +
> +// { dg-options "-std=c++17" }
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {my_var}
> {^\.(char|float)section$} } }
> +
> +template<class>
> +struct s
> +{
> + static int my_var;
> +};
> +
> +// { dg-final { scan-assembler-symbol-section {ZN1sIcE6my_varE}
> {^\.charsection$} } }
> +template<>
> +int
> +s<char>::my_var __attribute__((section(".charsection"))) = 1;
> +
> +// { dg-final { scan-assembler-symbol-section {ZN1sIfE6my_varE}
> {^\.floatsection$} } }
> +template<>
> +int
> +s<float>::my_var __attribute__((section(".floatsection"))) = 2;
> +
> +int *
> +f(bool which)
> +{
> + return which ? &s<char>::my_var : &s<float>::my_var;
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable-template.C
> gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable-template.C
> new file mode 100644
> index 00000000000..bef0f79064a
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable-template.C
> @@ -0,0 +1,19 @@
> +// attribute((section)) should affect static inline variable templates in class
> +// templates.
> +
> +// { dg-options "-std=c++17" }
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {my_var} {^\.testsection} } }
> +
> +template<class>
> +struct s
> +{
> + template<class>
> + inline static int my_var __attribute__((section(".testsection"))) = 1;
> +};
> +
> +int *
> +f(bool which)
> +{
> + return which ? &s<char>::my_var<char> : &s<float>::my_var<float>;
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable.C
> gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable.C
> new file mode 100644
> index 00000000000..359b79b6247
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-class-template-static-inline-variable.C
> @@ -0,0 +1,19 @@
> +// PR c++/70435
> +// attribute((section)) should affect static inline variables in class
> +// templates.
> +
> +// { dg-options "-std=c++17" }
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {my_var} {^\.testsection} } }
> +
> +template<class>
> +struct s
> +{
> + inline static int my_var __attribute__((section(".testsection"))) = 1;
> +};
> +
> +int *
> +f(bool which)
> +{
> + return which ? &s<char>::my_var : &s<float>::my_var;
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-class-template-static-variable.C
> gcc/testsuite/g++.dg/ext/section-class-template-static-variable.C
> new file mode 100644
> index 00000000000..77d742cc746
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-class-template-static-variable.C
> @@ -0,0 +1,20 @@
> +// attribute((section)) should affect static variables in class templates.
> +
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {my_var} {^\.testsection$} } }
> +
> +template<class>
> +struct s
> +{
> + static int my_var;
> +};
> +
> +template<class T>
> +int
> +s<T>::my_var __attribute__((section(".testsection"))) = 1;
> +
> +int *
> +f(bool which)
> +{
> + return which ? &s<char>::my_var : &s<float>::my_var;
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-function-template-static-variable-lto.C
> gcc/testsuite/g++.dg/ext/section-function-template-static-variable-lto.C
> new file mode 100644
> index 00000000000..8b960a3cda3
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-function-template-static-variable-lto.C
> @@ -0,0 +1,19 @@
> +// PR c++/70435
> +// attribute((section)) should affect static variables in function templates
> +// even with -flto.
> +
> +// { dg-do link }
> +// { dg-require-effective-target lto }
> +// { dg-require-named-sections "" }
> +// { dg-options "-flto --save-temps" }
> +
> +// { dg-final { scan-lto-assembler-symbol-section {my_var}
> {^(\.testsection|__DATA,__testsection)$} } }
> +#include "section-function-template-static-variable.C"
> +
> +// { dg-final { cleanup-saved-temps } }
> +
> +int
> +main()
> +{
> + return *f(true);
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-function-template-static-variable.C
> gcc/testsuite/g++.dg/ext/section-function-template-static-variable.C
> new file mode 100644
> index 00000000000..fc039950a3f
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-function-template-static-variable.C
> @@ -0,0 +1,26 @@
> +// PR c++/70435
> +// attribute((section)) should affect static variables in function templates.
> +
> +// { dg-do compile }
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {my_var}
> {^(\.testsection|__DATA,__testsection)$} } }
> +
> +#if defined(__APPLE__)
> +#define TESTSECTION "__DATA,__testsection"
> +#else
> +#define TESTSECTION ".testsection"
> +#endif
> +
> +template<class>
> +int *
> +callee()
> +{
> + static int my_var __attribute__((section(TESTSECTION))) = 1;
> + return &my_var;
> +}
> +
> +int *
> +f(bool which)
> +{
> + return which ? callee<char>() : callee<float>();
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-function-template.C
> gcc/testsuite/g++.dg/ext/section-function-template.C
> new file mode 100644
> index 00000000000..623f88b2d8e
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-function-template.C
> @@ -0,0 +1,20 @@
> +// attribute((section)) should affect function templates.
> +
> +// { dg-do compile }
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {callee} {^\.testsection$} } }
> +
> +template<class>
> +__attribute__((section(".testsection"))) void
> +callee()
> +{
> +}
> +
> +void
> +f(bool which)
> +{
> + if (which)
> + callee<int>();
> + else
> + callee<float>();
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-multi-tu-template-main.C
> gcc/testsuite/g++.dg/ext/section-multi-tu-template-main.C
> new file mode 100644
> index 00000000000..58ddcba49da
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-multi-tu-template-main.C
> @@ -0,0 +1,43 @@
> +// attribute((section)) variables in templates should be
> +// shared across translation units.
> +
> +// { dg-do run }
> +// { dg-require-named-sections "" }
> +// { dg-additional-sources "section-multi-tu-template-other.C" }
> +// { dg-options "--save-temps -std=c++17" }
> +
> +// { dg-final { scan-symbol-section
> "section-multi-tu-template-main.s" {[^_]f_var}
> {^(\.myfsection|__DATA,__myfsection)$} } }
> +// { dg-final { scan-symbol-section
> "section-multi-tu-template-main.s" {[^_]s_var}
> {^(\.myssection|__DATA,__myssection)$} } }
> +
> +// { dg-final { cleanup-saved-temps } }
> +
> +#include "section-multi-tu-template.h"
> +
> +int *
> +get_main_f_var_int()
> +{
> + return f<int>();
> +}
> +
> +int *
> +get_main_s_var_int()
> +{
> + return &s<int>::s_var;
> +}
> +
> +float *
> +get_main_s_var_float()
> +{
> + return &s<float>::s_var;
> +}
> +
> +int main()
> +{
> + if (get_main_f_var_int() != get_other_f_var_int())
> + return 1;
> + if (get_main_s_var_int() != get_other_s_var_int())
> + return 2;
> + if (get_main_s_var_float() != get_other_s_var_float())
> + return 3;
> + return 0;
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-multi-tu-template-other.C
> gcc/testsuite/g++.dg/ext/section-multi-tu-template-other.C
> new file mode 100644
> index 00000000000..be4e7c19665
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-multi-tu-template-other.C
> @@ -0,0 +1,24 @@
> +// This file is part of the section-multi-tu-template-main.C test.
> +
> +// { dg-options "-std=c++17" }
> +// { dg-require-named-sections "" }
> +
> +#include "section-multi-tu-template.h"
> +
> +int *
> +get_other_f_var_int()
> +{
> + return f<int>();
> +}
> +
> +int *
> +get_other_s_var_int()
> +{
> + return &s<int>::s_var;
> +}
> +
> +float *
> +get_other_s_var_float()
> +{
> + return &s<float>::s_var;
> +}
> diff --git gcc/testsuite/g++.dg/ext/section-multi-tu-template.h
> gcc/testsuite/g++.dg/ext/section-multi-tu-template.h
> new file mode 100644
> index 00000000000..8bfcbc36f46
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-multi-tu-template.h
> @@ -0,0 +1,33 @@
> +// This file is part of the section-multi-tu-template-main.C test.
> +
> +#if defined(__APPLE__)
> +#define MYFSECTION "__DATA,__myfsection"
> +#define MYSSECTION "__DATA,__myssection"
> +#else
> +#define MYFSECTION ".myfsection"
> +#define MYSSECTION ".myssection"
> +#endif
> +
> +template<class T>
> +struct s
> +{
> + static inline T s_var __attribute__((section(MYSSECTION))) = 42;
> +};
> +
> +template<class T>
> +T *
> +f()
> +{
> + static T f_var __attribute__((section(MYFSECTION))) = 42;
> + return &f_var;
> +}
> +
> +// section-multi-tu-template-main.C
> +int *get_main_f_var_int();
> +int *get_main_s_var_int();
> +float *get_main_s_var_float();
> +
> +// section-multi-tu-template-other.C
> +int *get_other_f_var_int();
> +int *get_other_s_var_int();
> +float *get_other_s_var_float();
> diff --git gcc/testsuite/g++.dg/ext/section-variable-template.C
> gcc/testsuite/g++.dg/ext/section-variable-template.C
> new file mode 100644
> index 00000000000..8a1577b8880
> --- /dev/null
> +++ gcc/testsuite/g++.dg/ext/section-variable-template.C
> @@ -0,0 +1,16 @@
> +// PR c++/88061
> +// attribute((section)) should affect variable templates.
> +
> +// { dg-options "-std=c++17" }
> +// { dg-require-named-sections "" }
> +// { dg-final { scan-assembler-symbol-section {my_var} {^\.testsection$} } }
> +
> +template<class>
> +inline int
> +my_var __attribute__((section(".testsection"))) = 1;
> +
> +int *
> +f(bool which)
> +{
> + return which ? &my_var<char> : &my_var<float>;
> +}
> --
> 2.22.0
More information about the Gcc-patches
mailing list