[RFC] rs6000: -mreadonly-in-sdata (PR82411)

Message ID 61d0d76104e9cee5a413ae050772870bcb47d49e.1519768361.git.segher@kernel.crashing.org
State New
Headers show
Series
  • [RFC] rs6000: -mreadonly-in-sdata (PR82411)
Related show

Commit Message

Segher Boessenkool Feb. 27, 2018, 10:01 p.m.
This adds a new option -mreadonly-in-sdata (on by default) that
controls whether readonly data can be put in sdata.  (For EABI this
does nothing, readonly data is put in sdata2 as usual).

Kees, could you try this out with your use case?  Add the flag
-mno-readonly-in-sdata in your build scripts.  The patch is against
GCC trunk.

[ I'll write documentation if this works; backports (to GCC 7 and
  GCC 6) can happen as well. ]


Segher


---
 gcc/config/rs6000/rs6000.c                     | 5 +++++
 gcc/config/rs6000/sysv4.opt                    | 4 ++++
 gcc/testsuite/gcc.target/powerpc/ppc-sdata-2.c | 1 +
 3 files changed, 10 insertions(+)

-- 
1.8.3.1

Comments

Carl Love via Gcc-patches Feb. 28, 2018, 9:46 p.m. | #1
On Tue, Feb 27, 2018 at 2:01 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> This adds a new option -mreadonly-in-sdata (on by default) that

> controls whether readonly data can be put in sdata.  (For EABI this

> does nothing, readonly data is put in sdata2 as usual).


Cool! Thanks for working on this.

> Kees, could you try this out with your use case?  Add the flag

> -mno-readonly-in-sdata in your build scripts.  The patch is against

> GCC trunk.


I'm struggling to create a ppc cross compiler, otherwise I would be
happy to test this. :)

If you're able to build a ppc kernel and remove the "static" from
mm/rodata_test_data.c's rodata_test_data and see the rodata_test_data
variable not in .sdata, that should be sufficient. The case reported
was here:
https://lkml.org/lkml/2017/9/21/156

I'll continue trying to solve my cross build issue...

Thanks!

-Kees

-- 
Kees Cook
Pixel Security
Segher Boessenkool Feb. 28, 2018, 11:23 p.m. | #2
On Wed, Feb 28, 2018 at 01:46:33PM -0800, Kees Cook wrote:
> On Tue, Feb 27, 2018 at 2:01 PM, Segher Boessenkool

> <segher@kernel.crashing.org> wrote:

> > This adds a new option -mreadonly-in-sdata (on by default) that

> > controls whether readonly data can be put in sdata.  (For EABI this

> > does nothing, readonly data is put in sdata2 as usual).

> 

> Cool! Thanks for working on this.

> 

> > Kees, could you try this out with your use case?  Add the flag

> > -mno-readonly-in-sdata in your build scripts.  The patch is against

> > GCC trunk.

> 

> I'm struggling to create a ppc cross compiler, otherwise I would be

> happy to test this. :)


For compiling kernels you can use
<http://git.infradead.org/users/segher/buildall.git>

> If you're able to build a ppc kernel and remove the "static" from

> mm/rodata_test_data.c's rodata_test_data and see the rodata_test_data

> variable not in .sdata, that should be sufficient. The case reported

> was here:

> https://lkml.org/lkml/2017/9/21/156

> 

> I'll continue trying to solve my cross build issue...


I'm trying to figure out how to get that file to build at all :-)


Segher
Carl Love via Gcc-patches March 1, 2018, 12:15 a.m. | #3
On Wed, Feb 28, 2018 at 3:23 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Wed, Feb 28, 2018 at 01:46:33PM -0800, Kees Cook wrote:

>> On Tue, Feb 27, 2018 at 2:01 PM, Segher Boessenkool

>> <segher@kernel.crashing.org> wrote:

>> > This adds a new option -mreadonly-in-sdata (on by default) that

>> > controls whether readonly data can be put in sdata.  (For EABI this

>> > does nothing, readonly data is put in sdata2 as usual).

>>

>> Cool! Thanks for working on this.

>>

>> > Kees, could you try this out with your use case?  Add the flag

>> > -mno-readonly-in-sdata in your build scripts.  The patch is against

>> > GCC trunk.

>>

>> I'm struggling to create a ppc cross compiler, otherwise I would be

>> happy to test this. :)

>

> For compiling kernels you can use

> <http://git.infradead.org/users/segher/buildall.git>


Hah, yes, I had just asked around privately about cross building tools
and was aimed there only to discover you're the author. ;)

>> If you're able to build a ppc kernel and remove the "static" from

>> mm/rodata_test_data.c's rodata_test_data and see the rodata_test_data

>> variable not in .sdata, that should be sufficient. The case reported

>> was here:

>> https://lkml.org/lkml/2017/9/21/156

>>

>> I'll continue trying to solve my cross build issue...

>

> I'm trying to figure out how to get that file to build at all :-)


This works for me with my distro cross compiler:

$ git diff
diff --git a/mm/rodata_test.c b/mm/rodata_test.c
index d908c8769b48..6bb4deb12e78 100644
--- a/mm/rodata_test.c
+++ b/mm/rodata_test.c
@@ -14,7 +14,7 @@
 #include <linux/uaccess.h>
 #include <asm/sections.h>

-static const int rodata_test_data = 0xC3;
+const int rodata_test_data = 0xC3;

 void rodata_test(void)
 {

$ CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc make wii_defconfig
$ for i in CONFIG_STRICT_KERNEL_RWX \
           CONFIG_DEBUG_KERNEL \
           CONFIG_DEBUG_RODATA_TEST ; do
    CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc ./scripts/config -e $i
done

$ CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc make -j...
...
$ objdump -t mm/rodata_test.o | grep rodata_test_data
00000000 g     O .sdata 00000004 rodata_test_data


-Kees

-- 
Kees Cook
Pixel Security
Segher Boessenkool March 1, 2018, 12:39 a.m. | #4
On Wed, Feb 28, 2018 at 04:15:23PM -0800, Kees Cook wrote:
> On Wed, Feb 28, 2018 at 3:23 PM, Segher Boessenkool

> <segher@kernel.crashing.org> wrote:

> > I'm trying to figure out how to get that file to build at all :-)


> $ CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc make wii_defconfig

> $ for i in CONFIG_STRICT_KERNEL_RWX \

>            CONFIG_DEBUG_KERNEL \

>            CONFIG_DEBUG_RODATA_TEST ; do

>     CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc ./scripts/config -e $i

> done


I use ppc6xx_defconfig normally; I had to disable CONFIG_HIBERNATION
as well.

Without the new flag, rodata_test_data ends up in .sdata in the object
file and in .data in vmlinux.

Adding

===
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index ccd2556..31b9613 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -143,6 +143,8 @@ CFLAGS-$(CONFIG_PPC64)      += $(call cc-option,-mcmodel=med
 CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mno-pointers-to-nested-functions)
 CFLAGS-$(CONFIG_PPC32) := -ffixed-r2 $(MULTIPLEWORD)
 
+CFLAGS-$(CONFIG_PPC32)	+= $(call cc-option,-mno-readonly-in-sdata)
+
 ifeq ($(CONFIG_PPC_BOOK3S_64),y)
 CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power7,-mtune=power4)
 CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=power4
===

it ends up in .rodata in both.

It would be nice to have a more comprehensive test, but this will do; I'll
commit it tomorrow (will resend, with doc and changelog and all that).

Cheers,


Segher
Carl Love via Gcc-patches March 1, 2018, 12:43 a.m. | #5
On Wed, Feb 28, 2018 at 4:39 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Wed, Feb 28, 2018 at 04:15:23PM -0800, Kees Cook wrote:

>> On Wed, Feb 28, 2018 at 3:23 PM, Segher Boessenkool

>> <segher@kernel.crashing.org> wrote:

>> > I'm trying to figure out how to get that file to build at all :-)

>

>> $ CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc make wii_defconfig

>> $ for i in CONFIG_STRICT_KERNEL_RWX \

>>            CONFIG_DEBUG_KERNEL \

>>            CONFIG_DEBUG_RODATA_TEST ; do

>>     CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc ./scripts/config -e $i

>> done

>

> I use ppc6xx_defconfig normally; I had to disable CONFIG_HIBERNATION

> as well.

>

> Without the new flag, rodata_test_data ends up in .sdata in the object

> file and in .data in vmlinux.

>

> Adding

>

> ===

> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile

> index ccd2556..31b9613 100644

> --- a/arch/powerpc/Makefile

> +++ b/arch/powerpc/Makefile

> @@ -143,6 +143,8 @@ CFLAGS-$(CONFIG_PPC64)      += $(call cc-option,-mcmodel=med

>  CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mno-pointers-to-nested-functions)

>  CFLAGS-$(CONFIG_PPC32) := -ffixed-r2 $(MULTIPLEWORD)

>

> +CFLAGS-$(CONFIG_PPC32) += $(call cc-option,-mno-readonly-in-sdata)

> +

>  ifeq ($(CONFIG_PPC_BOOK3S_64),y)

>  CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power7,-mtune=power4)

>  CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=power4

> ===

>

> it ends up in .rodata in both.


Excellent! That's perfect. :)

> It would be nice to have a more comprehensive test, but this will do; I'll

> commit it tomorrow (will resend, with doc and changelog and all that).


Thank you! Do you want to send the Makefile fix upstream too, or
should I route that?

-Kees

-- 
Kees Cook
Pixel Security
Segher Boessenkool March 1, 2018, 12:54 a.m. | #6
On Wed, Feb 28, 2018 at 04:43:56PM -0800, Kees Cook wrote:
> On Wed, Feb 28, 2018 at 4:39 PM, Segher Boessenkool

> <segher@kernel.crashing.org> wrote:

> > On Wed, Feb 28, 2018 at 04:15:23PM -0800, Kees Cook wrote:

> >> On Wed, Feb 28, 2018 at 3:23 PM, Segher Boessenkool

> >> <segher@kernel.crashing.org> wrote:

> >> > I'm trying to figure out how to get that file to build at all :-)

> >

> >> $ CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc make wii_defconfig

> >> $ for i in CONFIG_STRICT_KERNEL_RWX \

> >>            CONFIG_DEBUG_KERNEL \

> >>            CONFIG_DEBUG_RODATA_TEST ; do

> >>     CROSS_COMPILE=powerpc-linux-gnu- ARCH=powerpc ./scripts/config -e $i

> >> done

> >

> > I use ppc6xx_defconfig normally; I had to disable CONFIG_HIBERNATION

> > as well.

> >

> > Without the new flag, rodata_test_data ends up in .sdata in the object

> > file and in .data in vmlinux.

> >

> > Adding

> >

> > ===

> > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile

> > index ccd2556..31b9613 100644

> > --- a/arch/powerpc/Makefile

> > +++ b/arch/powerpc/Makefile

> > @@ -143,6 +143,8 @@ CFLAGS-$(CONFIG_PPC64)      += $(call cc-option,-mcmodel=med

> >  CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mno-pointers-to-nested-functions)

> >  CFLAGS-$(CONFIG_PPC32) := -ffixed-r2 $(MULTIPLEWORD)

> >

> > +CFLAGS-$(CONFIG_PPC32) += $(call cc-option,-mno-readonly-in-sdata)

> > +

> >  ifeq ($(CONFIG_PPC_BOOK3S_64),y)

> >  CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power7,-mtune=power4)

> >  CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=power4

> > ===

> >

> > it ends up in .rodata in both.

> 

> Excellent! That's perfect. :)

> 

> > It would be nice to have a more comprehensive test, but this will do; I'll

> > commit it tomorrow (will resend, with doc and changelog and all that).

> 

> Thank you! Do you want to send the Makefile fix upstream too, or

> should I route that?


If you could?  Have my

Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>


if you want.

It'll be a while before all users have a compiler with the new flag, but
I'll backport it to GCC 7 and GCC 6 (the open release branches) to speed
that up a bit.


Segher

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index bd0a564..dbc1d79 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -32591,6 +32591,11 @@  rs6000_elf_in_small_data_p (const_tree decl)
     }
   else
     {
+      /* If we are told not to put readonly data in sdata, then don't.  */
+      if (TREE_READONLY (decl) && rs6000_sdata != SDATA_EABI
+	  && !rs6000_readonly_in_sdata)
+	return false;
+
       HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (decl));
 
       if (size > 0
diff --git a/gcc/config/rs6000/sysv4.opt b/gcc/config/rs6000/sysv4.opt
index 9534c1c..fb03c0a 100644
--- a/gcc/config/rs6000/sysv4.opt
+++ b/gcc/config/rs6000/sysv4.opt
@@ -27,6 +27,10 @@  msdata=
 Target RejectNegative Joined Var(rs6000_sdata_name)
 Select method for sdata handling.
 
+mreadonly-in-sdata
+Target Report Var(rs6000_readonly_in_sdata) Init(1) Save
+Allow readonly data in sdata.
+
 mtls-size=
 Target RejectNegative Joined Var(rs6000_tls_size) Enum(rs6000_tls_size)
 Specify bit size of immediate TLS offsets.
diff --git a/gcc/testsuite/gcc.target/powerpc/ppc-sdata-2.c b/gcc/testsuite/gcc.target/powerpc/ppc-sdata-2.c
index 570c81f..ee77456 100644
--- a/gcc/testsuite/gcc.target/powerpc/ppc-sdata-2.c
+++ b/gcc/testsuite/gcc.target/powerpc/ppc-sdata-2.c
@@ -5,6 +5,7 @@ 
 /* { dg-final { scan-assembler-not "\\.section\[ \t\]\\.sdata2," } } */
 /* { dg-final { scan-assembler "sdat@sdarel\\(13\\)" } } */
 /* { dg-final { scan-assembler "sdat2@sdarel\\(13\\)" } } */
+/* { dg-skip-if "" { *-*-* } { "-mno-readonly-in-sdata" } { "" } } */
 
 
 int sdat = 2;