This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFC: Adding non-PIC executable support to MIPS


[Redirecting to gcc-patches]

Hi Dan,

Had a look at the gcc patch.  First of all, I gather that:

  (a) the new mode is selected by "-mabicalls -fno-pic" and
  (b) you're using specs to make "-fpic" the default for "-mabicalls"
      on targets that don't support non-PIC abicalls.

Is that right?  If so, I think it has some drawbacks:

  - The usual target-independent command-line interface is that
    "-fno-pic" is the default, so people might expect "-mabicalls
    -fno-pic" to be the same as "-mabicalls".  It is at the moment
    but isn't after the patch.

    (I've dealt with companies that add default options to the command
    line because they believe it's safer, so I wouldn't be surprised to
    find "-fno-pic" being used in the field.)

    I'd like to keep the target-independent part of the interface
    the same if possible.

  - More importantly, "-fpic" and "-fPIC" generate code that is
    suitable for shared libraries; as far as internals go, they set
    both "flag_shlib" and "flag_pic".  Thus the current executable
    mode is deliberately _not_ the same as "-mabicalls -fpic";
    the current executable mode sets "flag_pic" but not "flag_shlib".

    One consequence of this is that the patch prevents inlining of
    non-static functions for n32 and n64 executables.  E.g. consider:

        int foo (void) { return 1; }
        int bar (void) { return foo () + 2; }

    compiled with "mips64-linux-gnu-gcc -mabi=n32 -O3".  foo does not
    bind locally after the patch, and cannot be inlined into bar.
    (FWIW, I applied the patch locally to verify this.)

Having a separate option would avoid these problems.  As mentioned
in the earlier thread, I think it makes sense to view the new option
as a modifier for "-mno-shared".  If it weren't, the answer to
questions like:

    What happens if I use "-mshared -mnew-option"?  Is it the
    same as "-mnew-option -mshared"?

would be less intuitive (IMO).  It also means that the compiler option
directly mirrors the sense of the configure option; it enables and
disables a feature _when the other flags allow it_.

(Although I picked "-mgnu-plts", to go with the configure option
"--with-gnu-plts", I don't think you should.  Pick something that
fits in with your --enable option.)

With that change, I think it'd be slightly clearer to call the
internal macros TARGET_ABICALLS_PIC0 and TARGET_ABICALLS_PIC2.
Using "PIC" on its own might give the impression that the macros
are still tied to the -fpic/-fPIC options, whereas the new names
would link them directly to the associated assembler directives.

Daniel Jacobowitz <dan@debian.org> writes:
> Index: gcc/config/mips/mips.md
> ===================================================================
> --- gcc/config/mips/mips.md	(revision 137143)
> +++ gcc/config/mips/mips.md	(working copy)
> @@ -4462,6 +4462,22 @@
>    [(set (match_operand:P 0 "register_operand" "=d")
>  	(const:P (unspec:P [(const_int 0)] UNSPEC_GP)))])
>  
> +;; Move the constant value of __gnu_local_gp (operand 1) into
> +;; operand 0, for non-PIC abicalls code.  All uses of the result
> +;; are explicit, so there's no need for unspec_volatile here.
> +(define_insn_and_split "loadgp_nonpic_<mode>"
> +  [(set (match_operand:P 0 "register_operand" "=d")
> +	(const:P (unspec:P [(match_operand 1 "" "")] UNSPEC_LOADGP)))]
> +  "TARGET_NONPIC_ABICALLS"
> +  "#"
> +  ""
> +  [(const_int 0)]
> +{
> +  mips_emit_move (operands[0], operands[1]);
> +  DONE;
> +}
> +  [(set_attr "length" "8")])

Can you change the comment to say:

;; Move the constant value of __gnu_local_gp (operand 1) into
;; operand 0, for non-PIC abicalls code.  Unlike other loadgp_*
;; instructions, the destination is a temporary register rather
;; than pic_offset_table_rtx.

Uses of the result are eventually explicit for loadgp_absolute_<mode>
too, so the old comment didn't really explain the difference.

Is the code noticeably worse if you emit the move directly?
Although having a single insn has historically helped in some cases,
I'd be surprised if it makes much difference now, for this particular
pattern.  We emit all other such moves directly, so it isn't immediately
clear why this one should be different.

FWIW, if you just emit a move, the LO_SUM insn should already get a REG_EQUAL
note saying that the result is equal to (symbol_ref "__gnu_local_gp").
And that constant satisfies LEGITIMATE_CONSTANT_P, whereas the constant
in the pattern above doesn't.  Having an illegitimate constant could
easily prevent some optimisations.

It might be better to rename mips16_gp_pseudo_rtx & co. if you're going
to use them for non-MIPS16 code.

> @@ -5827,11 +5843,12 @@
>  
>  ;; Restore the gp that we saved above.  Despite the earlier comment, it seems
>  ;; that older code did recalculate the gp from $25.  Continue to jump through
> -;; $25 for compatibility (we lose nothing by doing so).
> +;; $25 for compatibility (we lose nothing by doing so).  Similarly restore
> +;; $gp if we might be jumping to code which expects that.
>  
>  (define_expand "builtin_longjmp"
>    [(use (match_operand 0 "register_operand"))]
> -  "TARGET_USE_GOT"
> +  "TARGET_USE_GOT || TARGET_ABICALLS"
>  {
>    /* The elements of the buffer are, in order:  */
>    int W = GET_MODE_SIZE (Pmode);

Hmm.  If the comment is really accurate about not losing anything,
how about making this unconditional and protecting the GP restoration
bits with a new macro:

  #define TARGET_FUNCTION_LOCAL_GP (TARGET_ABICALLS || TARGET_RTP_PIC)

?

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]