Reviving SH FDPIC target

Reviving SH FDPIC target

[Rich Felker]

I've started work on reviving the FDPIC support patch for the SH
target, which was proposed upstream in 2010 then abandoned:

https://gcc.gnu.org/ml/gcc-patches/2010-08/msg01464.html

Right now I'm in the process of determining what parts can be applied
as-is to current gcc, and what parts need to be adapted or rewritten
to account for changes in gcc between the 4.5 era and now (5.2/6.0).

The original patch as posted contained a significant amount of changes
that were unrelated to FDPIC code generation but rather for adding
sh*-uclinux tuples, and some things that even look like they're
associated with the old bFLT format. I am omitting these parts for now
since I'm unfamiliar with the old uClinux stuff and it's unmaintained.
If anyone else wants to use it, I think it would make more sense
factored as a separate patch anyway.

The target I have in mind is SH-2/J2 with musl libc, but uClibc or
even glibc could be made to work with it. I will submit the patches
for musl support (basically, just hooking up the dynamic linker name
for fdpic) separately; I believe they'll need merging with the
already-pending musl support patch from Szabolcs Nagy.

One question I'd like to ask now in case it's a problem that takes a
while to work out -- is copyright assignment already handled for the
old patch? The contributors listed in it are (all codesourcery):

- Daniel Jacobowitz
- Joseph Myers
- Mark Shinwell
- Andrew Stubbs

Also, according to Joseph Myers, there was some unresolved
disagreement that stalled (and eventually sunk) the old patch, so if
anyone's still around who has objections to it, could you speak up and
let me know what's wrong? Kaz Kojima seems to have approved the patch
at the time so I'm confused what the issue was/is.

Rich

Re: Reviving SH FDPIC target

[Joseph Myers]

On Wed, 2 Sep 2015, Rich Felker wrote:

> Also, according to Joseph Myers, there was some unresolved
> disagreement that stalled (and eventually sunk) the old patch, so if
> anyone's still around who has objections to it, could you speak up and
> let me know what's wrong? Kaz Kojima seems to have approved the patch
> at the time so I'm confused what the issue was/is.

It's patch 1/3 (architecture-independent) that had the disagreement (and 
patch 3/3 depends on patch 1/3).

https://gcc.gnu.org/ml/gcc-patches/2010-08/msg01462.html

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Reviving SH FDPIC target

[Rich Felker]

On Wed, Sep 02, 2015 at 07:59:45PM +0000, Joseph Myers wrote:
> On Wed, 2 Sep 2015, Rich Felker wrote:
> 
> > Also, according to Joseph Myers, there was some unresolved
> > disagreement that stalled (and eventually sunk) the old patch, so if
> > anyone's still around who has objections to it, could you speak up and
> > let me know what's wrong? Kaz Kojima seems to have approved the patch
> > at the time so I'm confused what the issue was/is.
> 
> It's patch 1/3 (architecture-independent) that had the disagreement (and 
> patch 3/3 depends on patch 1/3).
> 
> https://gcc.gnu.org/ml/gcc-patches/2010-08/msg01462.html

So this is only for __fpscr_values? In that case I think the right
solution is just to follow up with getting rid of __fpscr_values, if
it's not already done:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60138
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53513

53513 is marked fixed, but I didn't follow up to confirm that the
actual problems I reported in 60138 are fixed; I'll do some more
research on this. But if all goes well, we can just drop 1/3.

Thanks for the quick reply!

Rich

Re: Reviving SH FDPIC target

[Rich Felker]

On Wed, Sep 02, 2015 at 05:05:35PM -0400, Rich Felker wrote:
> On Wed, Sep 02, 2015 at 07:59:45PM +0000, Joseph Myers wrote:
> > On Wed, 2 Sep 2015, Rich Felker wrote:
> > 
> > > Also, according to Joseph Myers, there was some unresolved
> > > disagreement that stalled (and eventually sunk) the old patch, so if
> > > anyone's still around who has objections to it, could you speak up and
> > > let me know what's wrong? Kaz Kojima seems to have approved the patch
> > > at the time so I'm confused what the issue was/is.
> > 
> > It's patch 1/3 (architecture-independent) that had the disagreement (and 
> > patch 3/3 depends on patch 1/3).
> > 
> > https://gcc.gnu.org/ml/gcc-patches/2010-08/msg01462.html
> 
> So this is only for __fpscr_values? In that case I think the right
> solution is just to follow up with getting rid of __fpscr_values, if
> it's not already done:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60138
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53513
> 
> 53513 is marked fixed, but I didn't follow up to confirm that the
> actual problems I reported in 60138 are fixed; I'll do some more
> research on this. But if all goes well, we can just drop 1/3.

I've confirmed that gcc 5.2 does not produce references to
__fpscr_values; instead, it does:

	mov.l	.L4,r3
	...
	sts	fpscr,r1
	xor	r3,r1
	lds	r1,fpscr
	...
.L4:
	.long	524288

So if __fpscr_values was the only reason for patch 1/3 in the FDPIC
patchset, I think we can safely drop it. And patch 2/3 was already
committed, so 3/3, the one I was originally looking at, seems to be
all we need. It was approved at the time, so I'll proceed with merging
it with 5.2.0.

Rich

Re: Reviving SH FDPIC target

[Joseph Myers]

On Wed, 2 Sep 2015, Rich Felker wrote:

> So if __fpscr_values was the only reason for patch 1/3 in the FDPIC
> patchset, I think we can safely drop it. And patch 2/3 was already
> committed, so 3/3, the one I was originally looking at, seems to be
> all we need. It was approved at the time, so I'll proceed with merging
> it with 5.2.0.

Well, obviously if trying dropping patch 1/3 you need to remove everything 
related to use_initial_val (the feature added in patch 1/3) from patch 
3/3.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Reviving SH FDPIC target

[Rich Felker]

On Thu, Sep 03, 2015 at 02:58:39PM +0000, Joseph Myers wrote:
> On Wed, 2 Sep 2015, Rich Felker wrote:
> 
> > So if __fpscr_values was the only reason for patch 1/3 in the FDPIC
> > patchset, I think we can safely drop it. And patch 2/3 was already
> > committed, so 3/3, the one I was originally looking at, seems to be
> > all we need. It was approved at the time, so I'll proceed with merging
> > it with 5.2.0.
> 
> Well, obviously if trying dropping patch 1/3 you need to remove everything 
> related to use_initial_val (the feature added in patch 1/3) from patch 
> 3/3.

As far as I can tell, the only "use" of use_initial_val is defining
the pseudo-instruction in the md file, which causes the code in patch
1/3 to use it. I see no other references to it. As I understand, the
breakage from not having it (in the original 4.5-era patch) would be
when introducing references to __fpscr_values later, and no longer
having the GOT pointer, but that code is gone now.

Rich

Re: Reviving SH FDPIC target

[Rich Felker]

On Thu, Sep 03, 2015 at 11:53:45AM -0400, Rich Felker wrote:
> On Thu, Sep 03, 2015 at 02:58:39PM +0000, Joseph Myers wrote:
> > On Wed, 2 Sep 2015, Rich Felker wrote:
> > 
> > > So if __fpscr_values was the only reason for patch 1/3 in the FDPIC
> > > patchset, I think we can safely drop it. And patch 2/3 was already
> > > committed, so 3/3, the one I was originally looking at, seems to be
> > > all we need. It was approved at the time, so I'll proceed with merging
> > > it with 5.2.0.
> > 
> > Well, obviously if trying dropping patch 1/3 you need to remove everything 
> > related to use_initial_val (the feature added in patch 1/3) from patch 
> > 3/3.
> 
> As far as I can tell, the only "use" of use_initial_val is defining
> the pseudo-instruction in the md file, which causes the code in patch
> 1/3 to use it. I see no other references to it. As I understand, the
> breakage from not having it (in the original 4.5-era patch) would be
> when introducing references to __fpscr_values later, and no longer
> having the GOT pointer, but that code is gone now.

I have this basically working -- obviously no heavy testing yet, and
the specs glue is not sufficient to make it practical for others to
try it yet, so it'll be a little longer til I have something to post.

One thing I've noticed that's odd is that gcc -mfdpic -fPIC produces
different (less efficient) code from just gcc -mfdpic, which seems
wrong, but agrees with sh.c which has a number of checks for flag_pic
not matched with a TARGET_FDPIC check. I'm thinking all of these
should either be flag_pic||TARGET_PIC or flag_pic&&!TARGET_FDPIC,
depending on whether the code applies to all PIC or is specific to the
non-FDPIC PIC model where r12 is call-saved. Does this sound correct?
I think we need spurious -fPIC to work (although it could be handled
with spec magic) and not pessimize code, since most library builds
will use -fPIC.

Rich

Re: Reviving SH FDPIC target

[Segher Boessenkool]

On Fri, Sep 04, 2015 at 04:16:40PM -0400, Rich Felker wrote:
> One thing I've noticed that's odd is that gcc -mfdpic -fPIC produces
> different (less efficient) code from just gcc -mfdpic, which seems
> wrong, but agrees with sh.c which has a number of checks for flag_pic
> not matched with a TARGET_FDPIC check.

Generic code tests flag_pic in important places as well.

> I'm thinking all of these
> should either be flag_pic||TARGET_PIC or flag_pic&&!TARGET_FDPIC,
> depending on whether the code applies to all PIC or is specific to the
> non-FDPIC PIC model where r12 is call-saved. Does this sound correct?
> I think we need spurious -fPIC to work (although it could be handled
> with spec magic) and not pessimize code, since most library builds
> will use -fPIC.

If you never want -fPIC (or -fpic) if fdpic is enabled, you can disable
it (in sh_option_override)?


Segher

Re: Reviving SH FDPIC target

[Rich Felker]

On Fri, Sep 04, 2015 at 06:04:15PM -0500, Segher Boessenkool wrote:
> On Fri, Sep 04, 2015 at 04:16:40PM -0400, Rich Felker wrote:
> > One thing I've noticed that's odd is that gcc -mfdpic -fPIC produces
> > different (less efficient) code from just gcc -mfdpic, which seems
> > wrong, but agrees with sh.c which has a number of checks for flag_pic
> > not matched with a TARGET_FDPIC check.
> 
> Generic code tests flag_pic in important places as well.

Hmm, these are probably correct: things like default TLS model,
whether external functions defined in the same TU are subject to
interposition, etc. So ignoring -fPIC would be incorrect, and in fact
the different/less-efficient codegen might be correct. But if -fPIE is
generating different code, that's probably a bug (or at least
unwanted).

> > I'm thinking all of these
> > should either be flag_pic||TARGET_PIC or flag_pic&&!TARGET_FDPIC,
> > depending on whether the code applies to all PIC or is specific to the
> > non-FDPIC PIC model where r12 is call-saved. Does this sound correct?
> > I think we need spurious -fPIC to work (although it could be handled
> > with spec magic) and not pessimize code, since most library builds
> > will use -fPIC.
> 
> If you never want -fPIC (or -fpic) if fdpic is enabled, you can disable
> it (in sh_option_override)?

Even if it weren't needed for the above reasons, having it generate an
error would be very problematic from a standpoint of being able to
build packages unmodified. So I probably just need to have all the
conditionals in sh.c right (checking either ||TARGET_FDPIC or
&&!TARGET_FDPIC).

Rich

Re: Reviving SH FDPIC target

[Rich Felker]

On Fri, Sep 04, 2015 at 04:16:40PM -0400, Rich Felker wrote:
> On Thu, Sep 03, 2015 at 11:53:45AM -0400, Rich Felker wrote:
> > On Thu, Sep 03, 2015 at 02:58:39PM +0000, Joseph Myers wrote:
> > > On Wed, 2 Sep 2015, Rich Felker wrote:
> > > 
> > > > So if __fpscr_values was the only reason for patch 1/3 in the FDPIC
> > > > patchset, I think we can safely drop it. And patch 2/3 was already
> > > > committed, so 3/3, the one I was originally looking at, seems to be
> > > > all we need. It was approved at the time, so I'll proceed with merging
> > > > it with 5.2.0.
> > > 
> > > Well, obviously if trying dropping patch 1/3 you need to remove everything 
> > > related to use_initial_val (the feature added in patch 1/3) from patch 
> > > 3/3.
> > 
> > As far as I can tell, the only "use" of use_initial_val is defining
> > the pseudo-instruction in the md file, which causes the code in patch
> > 1/3 to use it. I see no other references to it. As I understand, the
> > breakage from not having it (in the original 4.5-era patch) would be
> > when introducing references to __fpscr_values later, and no longer
> > having the GOT pointer, but that code is gone now.
> 
> I have this basically working -- obviously no heavy testing yet, and
> the specs glue is not sufficient to make it practical for others to
> try it yet, so it'll be a little longer til I have something to post.

I'm running into a problem with crtstuff.c: the references to
__TMC_END__, etc. end up using @GOTOFF relocations which the linker
then rejects. Is this a linker bug or something I need to fix in the
target code to avoid generating @GOTOFF relocations? Since crtbegin.o
has no information about what section __TMC_END__ will be in when it's
defined, I don't see a lot of options except to avoid using @GOTOFF
entirely for symbols that aren't defined in the same TU.

Rich

Re: Reviving SH FDPIC target

[Rich Felker]

On Thu, Sep 10, 2015 at 11:49:19PM -0400, Rich Felker wrote:
> On Fri, Sep 04, 2015 at 04:16:40PM -0400, Rich Felker wrote:
> > On Thu, Sep 03, 2015 at 11:53:45AM -0400, Rich Felker wrote:
> > > On Thu, Sep 03, 2015 at 02:58:39PM +0000, Joseph Myers wrote:
> > > > On Wed, 2 Sep 2015, Rich Felker wrote:
> > > > 
> > > > > So if __fpscr_values was the only reason for patch 1/3 in the FDPIC
> > > > > patchset, I think we can safely drop it. And patch 2/3 was already
> > > > > committed, so 3/3, the one I was originally looking at, seems to be
> > > > > all we need. It was approved at the time, so I'll proceed with merging
> > > > > it with 5.2.0.
> > > > 
> > > > Well, obviously if trying dropping patch 1/3 you need to remove everything 
> > > > related to use_initial_val (the feature added in patch 1/3) from patch 
> > > > 3/3.
> > > 
> > > As far as I can tell, the only "use" of use_initial_val is defining
> > > the pseudo-instruction in the md file, which causes the code in patch
> > > 1/3 to use it. I see no other references to it. As I understand, the
> > > breakage from not having it (in the original 4.5-era patch) would be
> > > when introducing references to __fpscr_values later, and no longer
> > > having the GOT pointer, but that code is gone now.
> > 
> > I have this basically working -- obviously no heavy testing yet, and
> > the specs glue is not sufficient to make it practical for others to
> > try it yet, so it'll be a little longer til I have something to post.
> 
> I'm running into a problem with crtstuff.c: the references to
> __TMC_END__, etc. end up using @GOTOFF relocations which the linker
> then rejects. Is this a linker bug or something I need to fix in the
> target code to avoid generating @GOTOFF relocations? Since crtbegin.o
> has no information about what section __TMC_END__ will be in when it's
> defined, I don't see a lot of options except to avoid using @GOTOFF
> entirely for symbols that aren't defined in the same TU.

Well, that was easier than I thought, at least for a quick hack: I
added:

  SYMBOL_REF_EXTERNAL_P(orig) || DECL_SECTION_NAME(SYMBOL_REF_DECL(orig))

to the list of conditions for using @GOT instead of @GOTOFF. This may
actually be optimal under the constraints we have...

Rich

Re: Reviving SH FDPIC target

[Rich Felker]

On Fri, Sep 04, 2015 at 06:04:15PM -0500, Segher Boessenkool wrote:
> On Fri, Sep 04, 2015 at 04:16:40PM -0400, Rich Felker wrote:
> > One thing I've noticed that's odd is that gcc -mfdpic -fPIC produces
> > different (less efficient) code from just gcc -mfdpic, which seems
> > wrong, but agrees with sh.c which has a number of checks for flag_pic
> > not matched with a TARGET_FDPIC check.
> 
> Generic code tests flag_pic in important places as well.
> 
> > I'm thinking all of these
> > should either be flag_pic||TARGET_PIC or flag_pic&&!TARGET_FDPIC,
> > depending on whether the code applies to all PIC or is specific to the
> > non-FDPIC PIC model where r12 is call-saved. Does this sound correct?
> > I think we need spurious -fPIC to work (although it could be handled
> > with spec magic) and not pessimize code, since most library builds
> > will use -fPIC.
> 
> If you never want -fPIC (or -fpic) if fdpic is enabled, you can disable
> it (in sh_option_override)?

It turns out that with !flag_pic, gcc is generating broken code and/or
ICE, and this happens even after changing all the remaining flag_pic
tests in sh.c to flag_pic||TARGET_FDPIC. There are a few more in sh.md
I didn't try changing but they did not look relevant; the ICE came
via expand_binop in prepare_move_operands. Before I look at it
further, is there any reason to expect !flag_pic in the generic code
to break things when the target-specific code has PIC-like
constraints?

For now I just made sh_option_override force flag_pic when
TARGET_FDPIC is set. Note that flag_pic by itself is equivalent to
-fPIE; -fPIC also sets flag_shlib which affects other things like TLS
model and binds_locally interpretation. This seems like a viable
solution (and I got rid of the suboptimal codegen by fixing the
condition in sh_function_ok_for_sibcall so that flag_pic doesn't
preclude sibcalls when TARGET_FDPIC is set) but I'd still like to
figure out why gcc is breaking without flag_pic...

Rich