[PATCH v2] x86: Restore PC16 relocation overflow check

[H.J. Lu]

On Mon, May 31, 2021 at 6:04 AM Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Mon, 31 May 2021, H.J. Lu wrote:
>
> > > > > We can change the psABI.  But changes should make sense in the
> > > > > abstract as well as being demanded by reality.  I think we have good
> > > > > cause to make PC16 conforming now, but I don't think we have good
> > > > > cause to make its semantic anything else from the obvious.  FWIW, my
> > > > > patch would be
> > > > >   https://gitlab.com/x86-psABIs/x86-64-ABI/-/merge_requests/22
> > > > >
> > > > > Do you also want _8 and _16 (zero-extended I guess)?  _PC8
> > > > > (sign-extended)?
> > > > >
> > > >
> > > > Since there are 16-bit applications which expect PC16 behavior in
> > > > binutils 2.36, we can't change it without breaking them.
> > >
> > > Which change specifically would break them?  AFAICS any change in binutils
> > > will still result in the same binary output, so can you please explain?
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=27905
> >
> > > (I thought we are only talking about giving a meaning to PC16 in order to
> > > be able to make the linker depend on and hence check certain invariants)
> > >
> > > > We can update psABI to add a new PC16 relocation and leave the current
> > > > PC16 alone.
> > >
> > > Well, the current psABI makes PC16 use non-conforming, so strictly
> > > speaking we can do there whatever we want.  And even if that note of
> > > non-conformity were simply removed without any other change, it would
> > > still suggest a signed number (because, well, it's PC- _relative_, hence
> > > offset, hence signed).
> > >
> > > So, again, what will break if we make it explicit that it's a
> > > sign-extended number?
> >
> > It is not about the sign extension.
>
> I will say it is, in so far as when sign-extending it overflows, whereas
> with zero-extension it doesn't.  So we have the typical case of a 1.5
> range value indeed, and can't require just sign-extension.
>
> > It is about overflow behavior.  In 16-bit mode, PC16 can wrap around.
>
> So, let's look at the example you had in the above PR:
>
> [hjl@gnu-cfl-2 pr27905]$ cat rom.s
>         .code16gcc
>         .text
>         .section        .text.default_process_op.isra.0,"ax",@progbits
> default_process_op.isra.0:
>         ret
>         .section        .text.mpt_scsi_process_op,"ax",@progbits
> mpt_scsi_process_op:
>         jmp     default_process_op.isra.0
> ...
> .text.default_process_op.isra.0 0x737c : { *(.text.default_process_op.isra.0) }
> .text.mpt_scsi_process_op 0xf869 : { *(.text.mpt_scsi_process_op) }
>
> The offset between both sections is 0x84ed, which indeed can't be
> sign-extended to the same value in 16 bit.  But with wrap-around (and
> hence non-extension) it of course fits.  A real overflow would occur
> when the sections would be, say, 0x12345 bytes apart.

16-bit programs can't have 0x12345 displacement.

> So, what we can say in the psABI is that the value should match the
> original value when either sign- or zero-extended.  That still rules out
> "real" overflows (and checking for that makes sense, because also at
> runtime this won't work in any mode), but allows for this vague-extension
> to be relied upon.  Would that work for you?

The issue is how linker should handle overflow for PC16.  The current
linker assumes that PC16 is only used in 16-bit programs.  Of course, it
is wrong for 32-bit or 64-bit programs.

> (FWIW: I don't think we should add a new PC16 variant, if it only allows
> for stricter checking; we should clarify what we have and make it allow
> exactly what we need but not more; an psABI change with it's usual
> repercussions just for checking purposes seems to be too much).
>
>
> Ciao,
> Michael.



-- 
H.J.