On Wed, Oct 21, 2015 at 10:17:51PM +0900, Oleg Endo wrote:
> Rich,
>
> Thanks for the updated patch.
> Please do not start new threads for a continuation of an existing
> thread. This makes it difficult to track in the archives.
>
> On Tue, 2015-10-20 at 23:41 -0400, Rich Felker wrote:
> > Attached is a hopefully near-ready-for-commit version of the SH/FDPIC
> > patch. I believe I've addressed all comments by Oleg and Kaz on the
> > previous versions of the patch. I'm still working on drafting the
> > Changelog entry (there's a lot to go in it, and I might very well be
> > going into more detail than is needed).
>
> Other than the missing ChangeLog: How did you test the patch?
I've tested the new functionality (FDPIC) building musl, busybox,
dropbear, and other software. I believe Kaz tested that sh4 had no
regressions in the gcc tests with the patch applied.
> > One thing I've considered doing, since TARGET_FDPIC implies flag_pic
> > now, is removing all parts of the patch that just replace checks for
> > flag_pic with (flag_pic || TARGET_FDPIC). Would doing this be
> > desirable? It shrinks the patch a bit but of course more strongly
> > codes the assumption that TARGET_FDPIC implies flag_pic.
>
> If FDPIC only ever will make sense in combination with flag_pic != 0,
> then I guess this could be done.
The original patch did not force flag_pic, but I was getting broken
codegen and/or ICE without it and couldn't track down the source. In
any case, FDPIC _is_ position-independent code (an even more
constrained variant of it) so it makes sense for flag_pic to be set, I
think.
> If you do that, please add a comment
> above this hunk:
>
> > + if (TARGET_FDPIC && !flag_pic)
> > + flag_pic = 2;
OK, if I make this change I'll do that.
> Some other nits:
>
> > rtx lab = function_symbol (func_addr_rtx, "__movmemSI12_i4",
> > SFUNC_STATIC).lab;
>
> Break overlong lines to fit into 80 columns. E.g.
>
> rtx lab = function_symbol (func_addr_rtx, "__movmemSI12_i4",
> SFUNC_STATIC).lab;
OK. For some of these I wasn't sure of the proper style for wrapping
them so I figured I'd just wait to see what you say.
> > if (TARGET_FDPIC
> > && (TARGET_SHMEDIA || TARGET_SHCOMPACT || !TARGET_SH2))
> > sorry ("non-SH2 FDPIC");
>
> Drop SH5 stuff.
Without this passing -mfdpic on sh5 will probably lead to ICE or very
bogus codegen since there are lots of places that assume FDPIC and sh5
are mutually exclusive. If you're ok with that I can drop it, though.
> > if (TARGET_FDPIC)
> > {
> > emit_move_insn (gen_rtx_REG (Pmode, PIC_REG),
> > sh_get_fdpic_reg_initial_val ());
> > }
> >
>
> Remove braces around single statements.
OK. I prefer that too but I wasn't sure what style GCC favors.
> > return (GET_CODE (x) != CONST_DOUBLE
> > || mode == DFmode || mode == SFmode
> > || mode == DImode || GET_MODE (x) == VOIDmode);
>
> Remove unnecessary parens around return statements.
OK,
> When applying the patch I'm getting:
> patching file gcc/config/sh/sh-protos.h
> (Stripping trailing CRs from patch; use --binary to disable.)
>
> Maybe something with your editor settings?
I have my editor (Emacs) setup to always force unix text mode, so any
CR's show up as an editable/deletable ^M in the buffer, and I can't
find any. Grepping for literal CRs also fails. Is it possible the mail
system botched this?
Rich
