[v2] x86: Restore PC16 relocation overflow check

Message ID 20210528122245.2167858-1-hjl.tools@gmail.com
State New
Headers show
Series
  • [v2] x86: Restore PC16 relocation overflow check
Related show

Commit Message

Alan Modra via Binutils May 28, 2021, 12:22 p.m.
The x86-64 psABI has

---
A program or object file using R_X86_64_8, R_X86_64_16, R_X86_64_PC16
or R_X86_64_PC8 relocations is not conformant to this ABI, these
relocations are only added for documentation purposes.
---

Since x86 PC16 relocations were intended for 16-bit programs in an ELF32
or ELF64 container, PC16 relocation should wrap-around in 16-bit address
space.  Revert

commit a7664973b24a242cd9ea17deb5eaf503065fc0bd
Author: Jan Beulich <jbeulich@suse.com>
Date:   Mon Apr 26 10:41:35 2021 +0200

    x86: correct overflow checking for 16-bit PC-relative relocs

and xfail the related tests.  Also revert

commit 50c95a739c91ae70cf8481936611aa1f5397a384
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed May 26 12:13:13 2021 -0700

    x86: Propery check PC16 reloc overflow in 16-bit mode instructions

while keeping PR ld/27905 tests for PC16 relocation in 16-bit programs.

bfd/

	PR ld/27905
	* elf32-i386.c: Don't include "libiberty.h".
	(elf_howto_table): Revert commits a7664973b24 and 50c95a739c9.
	(elf_i386_rtype_to_howto): Revert commit 50c95a739c9.
	(elf_i386_info_to_howto_rel): Likewise.
	(elf_i386_tls_transition): Likewise.
	(elf_i386_relocate_section): Likewise.
	* elf64-x86-64.c (x86_64_elf_howto_table): Revert commits
	a7664973b24 and 50c95a739c9.
	(elf_x86_64_rtype_to_howto): Revert commit 50c95a739c9.
	* elfxx-x86.c (_bfd_x86_elf_parse_gnu_properties): Likewise.
	* elfxx-x86.h (elf_x86_obj_tdata): Likewise.
	(elf_x86_has_code16): Likewise.

binutils/

	PR ld/27905
	* readelf.c (decode_x86_feature_2): Revert commit 50c95a739c9.

gas/

	PR ld/27905
	* config/tc-i386.c (set_code_flag): Revert commit 50c95a739c9.
	(set_16bit_gcc_code_flag): Likewise.
	(x86_cleanup): Likewise.
	* testsuite/gas/i386/code16-2.d: Updated.
	* testsuite/gas/i386/x86-64-code16-2.d: Likewise.

include/

	PR ld/27905
	* elf/common.h (GNU_PROPERTY_X86_FEATURE_2_CODE16): Removed.

ld/

	PR ld/27905
	* testsuite/ld-i386/pcrel16-2.d: xfail.
	* testsuite/ld-x86-64/pcrel16-2.d: Likewise.
---
 bfd/elf32-i386.c                         | 25 +++--------
 bfd/elf64-x86-64.c                       | 14 +-----
 bfd/elfxx-x86.c                          |  9 +---
 bfd/elfxx-x86.h                          |  6 ---
 binutils/readelf.c                       |  3 --
 gas/config/tc-i386.c                     | 55 ++++++++----------------
 gas/testsuite/gas/i386/code16-2.d        |  5 ---
 gas/testsuite/gas/i386/x86-64-code16-2.d |  5 ---
 include/elf/common.h                     |  1 -
 ld/testsuite/ld-i386/pcrel16-2.d         |  1 +
 ld/testsuite/ld-x86-64/pcrel16-2.d       |  1 +
 11 files changed, 29 insertions(+), 96 deletions(-)

-- 
2.31.1

Comments

Alan Modra via Binutils May 28, 2021, 12:41 p.m. | #1
On 28.05.2021 14:22, H.J. Lu wrote:
> The x86-64 psABI has

> 

> ---

> A program or object file using R_X86_64_8, R_X86_64_16, R_X86_64_PC16

> or R_X86_64_PC8 relocations is not conformant to this ABI, these

> relocations are only added for documentation purposes.

> ---

> 

> Since x86 PC16 relocations were intended for 16-bit programs in an ELF32

> or ELF64 container, PC16 relocation should wrap-around in 16-bit address

> space.


At the risk of stating the obvious - this wrapping around is not
expressed correctly by complain_overflow_bitfield. I simply can't
see how, in a 32- or 64-bit container, 16-bit code could ever be
linked correctly, suitably detecting overflow where it would
cause a problem at runtime. This is only possible when the linker
knows 16-bit segment boundaries.

Therefore I see only two reasonable modes of operation for the
linker:
1) Treat PC16 analogously to PC8/PC32/PC64.
2) Don't check PC16 (and then also PC8, 8, and 16) for overflow
   at all.
The default ought to be 1, while 2 ought to be enabled explicitly
(and at the programmer's own risk) for code caring to link sizable
amounts of 16-bit code (i.e. when the overflow checking gets in
the way).
This said, with the psABI saying what it says, I could see one
taking the position of taking "don't check overflows at all" as
a legitimate mode to always operate in. But then again this
ought to apply to all four non-conformant reloc types at the
same time.

Jan
Michael Matz May 28, 2021, 3:52 p.m. | #2
Hello,

On Fri, 28 May 2021, Jan Beulich via Binutils wrote:

> On 28.05.2021 14:22, H.J. Lu wrote:

> > The x86-64 psABI has

> > 

> > ---

> > A program or object file using R_X86_64_8, R_X86_64_16, R_X86_64_PC16

> > or R_X86_64_PC8 relocations is not conformant to this ABI, these

> > relocations are only added for documentation purposes.

> > ---

> > 

> > Since x86 PC16 relocations were intended for 16-bit programs in an ELF32

> > or ELF64 container, PC16 relocation should wrap-around in 16-bit address

> > space.

> 

> At the risk of stating the obvious - this wrapping around is not

> expressed correctly by complain_overflow_bitfield. I simply can't

> see how, in a 32- or 64-bit container, 16-bit code could ever be

> linked correctly, suitably detecting overflow where it would

> cause a problem at runtime. This is only possible when the linker

> knows 16-bit segment boundaries.

> 

> Therefore I see only two reasonable modes of operation for the

> linker:

> 1) Treat PC16 analogously to PC8/PC32/PC64.


I think this is the right course of action.  PC16 should check that the 
signed 16bit value is the same as the computed 64 bit (or 32bit on ELF32) 
value that's supposed to go in there.

The PC16 (or any of the other relocations named PC*) is not the place to 
check for the funny truncation of rIP that some instructions are doing.  
PC* has uses outside of instructions, and even within instructions the 
"normal" meaning makes sense and reflects reality the most (i.e. no rIP 
truncation).

If we _really_ wanted that linkers be able to check that rIP truncations 
is correct then that would need a new relocation.  I will remark that 
checking such truncation would require the linker to know where the final 
linked runtime address is, and that in the wild, for loaders and BIOSes 
writing into ELF files for merely containing byte blobs to be later 
extracted by objcopy with the assumption that that blob then is magically 
loaded at "the right address", these final load addresses are often _not_ 
written into the linker scripts correctly.  So, in the end, even if we 
were to introduce such new relocation with the rIP truncation checking, it 
often would have to be disabled anyway in practice because the checks then 
would trigger.

> 2) Don't check PC16 (and then also PC8, 8, and 16) for overflow

>    at all.

> The default ought to be 1, while 2 ought to be enabled explicitly

> (and at the programmer's own risk) for code caring to link sizable

> amounts of 16-bit code (i.e. when the overflow checking gets in

> the way).

> This said, with the psABI saying what it says, I could see one

> taking the position of taking "don't check overflows at all" as

> a legitimate mode to always operate in. But then again this

> ought to apply to all four non-conformant reloc types at the

> same time.


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)?


Ciao,
Michael.
Alan Modra via Binutils May 28, 2021, 4:23 p.m. | #3
On Fri, May 28, 2021 at 8:52 AM Michael Matz <matz@suse.de> wrote:
>

> Hello,

>

> On Fri, 28 May 2021, Jan Beulich via Binutils wrote:

>

> > On 28.05.2021 14:22, H.J. Lu wrote:

> > > The x86-64 psABI has

> > >

> > > ---

> > > A program or object file using R_X86_64_8, R_X86_64_16, R_X86_64_PC16

> > > or R_X86_64_PC8 relocations is not conformant to this ABI, these

> > > relocations are only added for documentation purposes.

> > > ---

> > >

> > > Since x86 PC16 relocations were intended for 16-bit programs in an ELF32

> > > or ELF64 container, PC16 relocation should wrap-around in 16-bit address

> > > space.

> >

> > At the risk of stating the obvious - this wrapping around is not

> > expressed correctly by complain_overflow_bitfield. I simply can't

> > see how, in a 32- or 64-bit container, 16-bit code could ever be

> > linked correctly, suitably detecting overflow where it would

> > cause a problem at runtime. This is only possible when the linker

> > knows 16-bit segment boundaries.

> >

> > Therefore I see only two reasonable modes of operation for the

> > linker:

> > 1) Treat PC16 analogously to PC8/PC32/PC64.

>

> I think this is the right course of action.  PC16 should check that the

> signed 16bit value is the same as the computed 64 bit (or 32bit on ELF32)

> value that's supposed to go in there.

>

> The PC16 (or any of the other relocations named PC*) is not the place to

> check for the funny truncation of rIP that some instructions are doing.

> PC* has uses outside of instructions, and even within instructions the

> "normal" meaning makes sense and reflects reality the most (i.e. no rIP

> truncation).

>

> If we _really_ wanted that linkers be able to check that rIP truncations

> is correct then that would need a new relocation.  I will remark that

> checking such truncation would require the linker to know where the final

> linked runtime address is, and that in the wild, for loaders and BIOSes

> writing into ELF files for merely containing byte blobs to be later

> extracted by objcopy with the assumption that that blob then is magically

> loaded at "the right address", these final load addresses are often _not_

> written into the linker scripts correctly.  So, in the end, even if we

> were to introduce such new relocation with the rIP truncation checking, it

> often would have to be disabled anyway in practice because the checks then

> would trigger.

>

> > 2) Don't check PC16 (and then also PC8, 8, and 16) for overflow

> >    at all.

> > The default ought to be 1, while 2 ought to be enabled explicitly

> > (and at the programmer's own risk) for code caring to link sizable

> > amounts of 16-bit code (i.e. when the overflow checking gets in

> > the way).

> > This said, with the psABI saying what it says, I could see one

> > taking the position of taking "don't check overflows at all" as

> > a legitimate mode to always operate in. But then again this

> > ought to apply to all four non-conformant reloc types at the

> > same time.

>

> 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.  We can
update psABI to add a new PC16 relocation and leave the current
PC16 alone.


-- 
H.J.
Alan Modra via Binutils May 28, 2021, 4:23 p.m. | #4
On 28.05.2021 17:52, Michael Matz 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)?


_PC8 - Yes, please. I'm less certain about _8 and _16, but if you want to
also "promote" them, then I think they should follow _32, or - short of
having a signed variant - be flexible to allow for both sign- or zero-
extension (i.e. what in binutils check_overflow_bitfield effects).

Jan
Michael Matz May 31, 2021, 12:27 p.m. | #5
Hello,

On Fri, 28 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?

(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?


Ciao,
Michael.
Alan Modra via Binutils May 31, 2021, 12:45 p.m. | #6
On Mon, May 31, 2021 at 5:27 AM Michael Matz <matz@suse.de> wrote:
>

> Hello,

>

> On Fri, 28 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.  It is about overflow behavior.
In 16-bit mode, PC16 can wrap around.

-- 
H.J.
Michael Matz May 31, 2021, 1:04 p.m. | #7
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.

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?

(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.
Alan Modra via Binutils May 31, 2021, 1:24 p.m. | #8
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.
Alan Modra via Binutils May 31, 2021, 2:19 p.m. | #9
On 31.05.2021 15:04, Michael Matz 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 isn't just 1.5, is it? A negative offset of -0xffff will similarly
wrap on hardware in 16-bit mode ...

Jan
Michael Matz May 31, 2021, 2:36 p.m. | #10
Hello,

On Mon, 31 May 2021, H.J. Lu wrote:

> > > > 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.


Which is exactly why _that_ should be flagged as error by the linker as a 
real overflow.  Whereas a both-extension that preserves the value is 
acceptable in the right mode (and hence should not be flagged by the 
linker as a problem).

This is basically what I would like the psABI to say, displacements from 
-0xffff to 0xffff are acceptable, larger or smaller ones aren't.

Do you agree with that?

> > 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.


I think as per above.  An value within [-0xffff,0xffff] (mathematically, 
i.e. the 32 or 64 bit value of the computation S+A-P) is defined as not 
overflowing.  The linker basically needs to assume that the author knew 
what he was doing when using a PC16 offset and only needs to flag things 
that simply cannot be made to work (like e.g. jumping 0x12345 bytes 
forward).

> The current linker assumes that PC16 is only used in 16-bit programs.  


Right, and it should continue doing so, and that acceptable use I would 
like to accept and encode in the psABI.

> Of course, it is wrong for 32-bit or 64-bit programs.



Ciao,
Michael.
Michael Matz May 31, 2021, 2:42 p.m. | #11
Hello,

On Mon, 31 May 2021, Jan Beulich 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 isn't just 1.5, is it? A negative offset of -0xffff will similarly

> wrap on hardware in 16-bit mode ...


Err, right, of course, underflow can also happen.  So the acceptable range 
of values is [-0xffff,0xffff] which is more than those values that are 
value-preserved under sign- or zero-extension.


Ciao,
Michael.
Alan Modra via Binutils May 31, 2021, 2:47 p.m. | #12
On 31.05.2021 16:36, Michael Matz wrote:
> On Mon, 31 May 2021, H.J. Lu wrote:

>>>>> 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.

> 

> Which is exactly why _that_ should be flagged as error by the linker as a 

> real overflow.  Whereas a both-extension that preserves the value is 

> acceptable in the right mode (and hence should not be flagged by the 

> linker as a problem).

> 

> This is basically what I would like the psABI to say, displacements from 

> -0xffff to 0xffff are acceptable, larger or smaller ones aren't.

> 

> Do you agree with that?

> 

>>> 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.

> 

> I think as per above.  An value within [-0xffff,0xffff] (mathematically, 

> i.e. the 32 or 64 bit value of the computation S+A-P) is defined as not 

> overflowing.  The linker basically needs to assume that the author knew 

> what he was doing when using a PC16 offset and only needs to flag things 

> that simply cannot be made to work (like e.g. jumping 0x12345 bytes 

> forward).


While the ABI is free to state what it wants, such a wide range is not
very useful when the relocation is used outside of 16-bit mode. The
main need for the overflow detection is when it's out of range by just
a little (e.g. because of some piece of code or data having grown just
enough to bring it out of range). This is what my change was about,
which - aiui - H.J. has meanwhile reverted.

Jan
Michael Matz May 31, 2021, 3:16 p.m. | #13
Hello,

On Mon, 31 May 2021, Jan Beulich wrote:

> > Which is exactly why _that_ should be flagged as error by the linker as a 

> > real overflow.  Whereas a both-extension that preserves the value is 

> > acceptable in the right mode (and hence should not be flagged by the 

> > linker as a problem).

> > 

> > This is basically what I would like the psABI to say, displacements from 

> > -0xffff to 0xffff are acceptable, larger or smaller ones aren't.

> > 

> > Do you agree with that?

> > 

> >>> 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.

> > 

> > I think as per above.  An value within [-0xffff,0xffff] (mathematically, 

> > i.e. the 32 or 64 bit value of the computation S+A-P) is defined as not 

> > overflowing.  The linker basically needs to assume that the author knew 

> > what he was doing when using a PC16 offset and only needs to flag things 

> > that simply cannot be made to work (like e.g. jumping 0x12345 bytes 

> > forward).

> 

> While the ABI is free to state what it wants, such a wide range is not 

> very useful when the relocation is used outside of 16-bit mode.


Well, as it stands the relocation needs to support uses within and outside 
16-bit mode, and the linker can't differ between those, so what's 
acceptable needs to be the union of all ranges.

> The main need for the overflow detection is when it's out of range by 

> just a little (e.g. because of some piece of code or data having grown 

> just enough to bring it out of range).


But out of _what_ range?  You are saying that [-0xffff,0xffff] is too 
broad, what range do you want?  (I don't think you can get away with a 
smaller range with PC16 in the x86-64 psABI, but for completeness)

> This is what my change was about, which - aiui - H.J. has meanwhile 

> reverted.


If your change flagged values within [0x8000,0xffff] or [-0xffff,-0x8001] 
by default then I think the reversal was justified, those need to be 
continued to be accepted, for better or worse.


Ciao,
Michael.
Alan Modra via Binutils June 1, 2021, 6:19 a.m. | #14
On 31.05.2021 17:16, Michael Matz wrote:
> Hello,

> 

> On Mon, 31 May 2021, Jan Beulich wrote:

> 

>>> Which is exactly why _that_ should be flagged as error by the linker as a 

>>> real overflow.  Whereas a both-extension that preserves the value is 

>>> acceptable in the right mode (and hence should not be flagged by the 

>>> linker as a problem).

>>>

>>> This is basically what I would like the psABI to say, displacements from 

>>> -0xffff to 0xffff are acceptable, larger or smaller ones aren't.

>>>

>>> Do you agree with that?

>>>

>>>>> 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.

>>>

>>> I think as per above.  An value within [-0xffff,0xffff] (mathematically, 

>>> i.e. the 32 or 64 bit value of the computation S+A-P) is defined as not 

>>> overflowing.  The linker basically needs to assume that the author knew 

>>> what he was doing when using a PC16 offset and only needs to flag things 

>>> that simply cannot be made to work (like e.g. jumping 0x12345 bytes 

>>> forward).

>>

>> While the ABI is free to state what it wants, such a wide range is not 

>> very useful when the relocation is used outside of 16-bit mode.

> 

> Well, as it stands the relocation needs to support uses within and outside 

> 16-bit mode, and the linker can't differ between those, so what's 

> acceptable needs to be the union of all ranges.

> 

>> The main need for the overflow detection is when it's out of range by 

>> just a little (e.g. because of some piece of code or data having grown 

>> just enough to bring it out of range).

> 

> But out of _what_ range?  You are saying that [-0xffff,0xffff] is too 

> broad, what range do you want?  (I don't think you can get away with a 

> smaller range with PC16 in the x86-64 psABI, but for completeness)


The true disp16 range - [-0x8000,+0x7fff]. As said before, for the
purposes of 32- and 64-bit code (and data), this (or a new, proper
reloc type) needs to match PC32 allowing [-0x80000000,+0x7fffffff].
And obviously this also ought to match PC8, allowing [-0x80,+0x7f].

Also note how PC8's current handling in bfd does _not_ fit 16-bit
code (wrapping at the 64k boundary not being accounted for).

>> This is what my change was about, which - aiui - H.J. has meanwhile 

>> reverted.

> 

> If your change flagged values within [0x8000,0xffff] or [-0xffff,-0x8001] 

> by default then I think the reversal was justified, those need to be 

> continued to be accepted, for better or worse.


Interesting - I took your initial reply to H.J. to mean the opposite.
Obviously a misunderstanding then ...

Jan
Michael Matz June 1, 2021, 1:02 p.m. | #15
Hello,

On Tue, 1 Jun 2021, Jan Beulich wrote:

> >>>> The issue is how linker should handle overflow for PC16.

> >>>

> >>> I think as per above.  An value within [-0xffff,0xffff] 

> >>> (mathematically, i.e. the 32 or 64 bit value of the computation 

> >>> S+A-P) is defined as not overflowing.  The linker basically needs to 

> >>> assume that the author knew what he was doing when using a PC16 

> >>> offset and only needs to flag things that simply cannot be made to 

> >>> work (like e.g. jumping 0x12345 bytes forward).

> >>

> >> While the ABI is free to state what it wants, such a wide range is 

> >> not very useful when the relocation is used outside of 16-bit mode.

> > 

> > Well, as it stands the relocation needs to support uses within and 

> > outside 16-bit mode, and the linker can't differ between those, so 

> > what's acceptable needs to be the union of all ranges.

> > 

> >> The main need for the overflow detection is when it's out of range by 

> >> just a little (e.g. because of some piece of code or data having 

> >> grown just enough to bring it out of range).

> > 

> > But out of _what_ range?  You are saying that [-0xffff,0xffff] is too 

> > broad, what range do you want?  (I don't think you can get away with a 

> > smaller range with PC16 in the x86-64 psABI, but for completeness)

> 

> The true disp16 range - [-0x8000,+0x7fff].


Okay, well, yeah, I meanwhile think we can't do that generally :-/.  We 
could possibly with an option, and I think we could have that option on by 
default to be disabled by code that needs to embed 16bit code into elf32 
or elf64 binaries.  (That option would extend the accepted range to 
[-0xffff,0xffff])

> As said before, for the purposes of 32- and 64-bit code (and data), this 

> (or a new, proper reloc type) needs to match PC32 allowing 

> [-0x80000000,+0x7fffffff]. And obviously this also ought to match PC8, 

> allowing [-0x80,+0x7f].


I think for PC8 we have an easier time: there's no code model that wraps 
around at 256, so allowing only [-0x80,0x7f] now and requiring 
sign-extension to preserve the value ought to not give us any headaches in 
the wild.

> Also note how PC8's current handling in bfd does _not_ fit 16-bit

> code (wrapping at the 64k boundary not being accounted for).


I don't think we "correctly" do that with any code model or relocation, 
right?  The linker doesn't know the context of relocations, and hence 
can't know if it would need to account for wrap-around at 64k, at 4G or 
at 1<<64.  Or, if we want to get into extremes consider the A20 gate and a 
wraparound at 1M.  There's only so much we can model.

I mean you can embed 32bit code in elf64, and use an PC32 reloc to get 
from 3G to 0G, if you consider the wrap-around.  When only strictly 
accepting the signed32 range that would be impossible, and we have the 
same problem as with PC16: should we, or should we not, flag such use, and 
under which conditions?

In such cases we need to consider existing use in the wild and provide a 
way to accept existing .o files.  If those cases are seldom then possibly 
only under an option.

> >> This is what my change was about, which - aiui - H.J. has meanwhile 

> >> reverted.

> > 

> > If your change flagged values within [0x8000,0xffff] or [-0xffff,-0x8001] 

> > by default then I think the reversal was justified, those need to be 

> > continued to be accepted, for better or worse.

> 

> Interesting - I took your initial reply to H.J. to mean the opposite.

> Obviously a misunderstanding then ...


No, I changed my mind in the meantime.  I would have liked to say the 
obvious thing in the psABI and not consider wrap-around, but now think we 
can't go the full way for that.  I still would like us to make PC8 and 
PC16 acceptable, but the latter with a larger than obvious acceptable 
range.

As for a new relocaiton that does have the signed16 restriction, no matter 
the code model, I remain doubtful: it can introduce more (compatibility) 
problems than it solves.


Ciao,
Michael.
Alan Modra via Binutils June 1, 2021, 3:08 p.m. | #16
On 01.06.2021 15:02, Michael Matz wrote:
> On Tue, 1 Jun 2021, Jan Beulich wrote:

>>>>>> The issue is how linker should handle overflow for PC16.

>>>>>

>>>>> I think as per above.  An value within [-0xffff,0xffff] 

>>>>> (mathematically, i.e. the 32 or 64 bit value of the computation 

>>>>> S+A-P) is defined as not overflowing.  The linker basically needs to 

>>>>> assume that the author knew what he was doing when using a PC16 

>>>>> offset and only needs to flag things that simply cannot be made to 

>>>>> work (like e.g. jumping 0x12345 bytes forward).

>>>>

>>>> While the ABI is free to state what it wants, such a wide range is 

>>>> not very useful when the relocation is used outside of 16-bit mode.

>>>

>>> Well, as it stands the relocation needs to support uses within and 

>>> outside 16-bit mode, and the linker can't differ between those, so 

>>> what's acceptable needs to be the union of all ranges.

>>>

>>>> The main need for the overflow detection is when it's out of range by 

>>>> just a little (e.g. because of some piece of code or data having 

>>>> grown just enough to bring it out of range).

>>>

>>> But out of _what_ range?  You are saying that [-0xffff,0xffff] is too 

>>> broad, what range do you want?  (I don't think you can get away with a 

>>> smaller range with PC16 in the x86-64 psABI, but for completeness)

>>

>> The true disp16 range - [-0x8000,+0x7fff].

> 

> Okay, well, yeah, I meanwhile think we can't do that generally :-/.  We 

> could possibly with an option, and I think we could have that option on by 

> default to be disabled by code that needs to embed 16bit code into elf32 

> or elf64 binaries.  (That option would extend the accepted range to 

> [-0xffff,0xffff])


That's one of the things I proposed to H.J. earlier on. He took the
position that even such projects should continue to work "out-of-the-
box" with the next binutils release. I disagree, but he's the
maintainer ...

>> As said before, for the purposes of 32- and 64-bit code (and data), this 

>> (or a new, proper reloc type) needs to match PC32 allowing 

>> [-0x80000000,+0x7fffffff]. And obviously this also ought to match PC8, 

>> allowing [-0x80,+0x7f].

> 

> I think for PC8 we have an easier time: there's no code model that wraps 

> around at 256, so allowing only [-0x80,0x7f] now and requiring 

> sign-extension to preserve the value ought to not give us any headaches in 

> the wild.

> 

>> Also note how PC8's current handling in bfd does _not_ fit 16-bit

>> code (wrapping at the 64k boundary not being accounted for).

> 

> I don't think we "correctly" do that with any code model or relocation, 

> right?  The linker doesn't know the context of relocations, and hence 

> can't know if it would need to account for wrap-around at 64k, at 4G or 

> at 1<<64.  Or, if we want to get into extremes consider the A20 gate and a 

> wraparound at 1M.  There's only so much we can model.


You don't mention the A20 gate here, do you? ;-)

> I mean you can embed 32bit code in elf64, and use an PC32 reloc to get 

> from 3G to 0G, if you consider the wrap-around.  When only strictly 

> accepting the signed32 range that would be impossible, and we have the 

> same problem as with PC16: should we, or should we not, flag such use, and 

> under which conditions?


Right, I did consider this PC32 issues as well. Embedding smaller
address size code into an object targeting a larger address size
architecture is of course always problematic. Hence why I think the
PC32 handling is okay as is, but the PC16 one would better match in
how it gets treated.

> In such cases we need to consider existing use in the wild and provide a 

> way to accept existing .o files.  If those cases are seldom then possibly 

> only under an option.


Right - the tradeoff between sufficiently precise checks (with false
positive errors) and sufficiently imprecise checks (with false
negatives, i.e. missing diagnostics). That's generally something only
a human can decide, and only on a project by project or even component
by component basis.

>>>> This is what my change was about, which - aiui - H.J. has meanwhile 

>>>> reverted.

>>>

>>> If your change flagged values within [0x8000,0xffff] or [-0xffff,-0x8001] 

>>> by default then I think the reversal was justified, those need to be 

>>> continued to be accepted, for better or worse.

>>

>> Interesting - I took your initial reply to H.J. to mean the opposite.

>> Obviously a misunderstanding then ...

> 

> No, I changed my mind in the meantime.  I would have liked to say the 

> obvious thing in the psABI and not consider wrap-around, but now think we 

> can't go the full way for that.  I still would like us to make PC8 and 

> PC16 acceptable, but the latter with a larger than obvious acceptable 

> range.

> 

> As for a new relocaiton that does have the signed16 restriction, no matter 

> the code model, I remain doubtful: it can introduce more (compatibility) 

> problems than it solves.


Indeed, there is this risk. I continue to be in favor of a new command
line option, off by default, restoring original PC16 checking and at
the same time making PC32 in a 64-bit container match. (It could be two
separate options, but to me this feels like going too far.) As per what
you say above, I'd be happy to leave PC8 as is.

H.J. - can we get you to re-consider?

Jan
Alan Modra via Binutils June 1, 2021, 4:29 p.m. | #17
On Tue, Jun 1, 2021 at 8:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 01.06.2021 15:02, Michael Matz wrote:

> > On Tue, 1 Jun 2021, Jan Beulich wrote:

> >>>>>> The issue is how linker should handle overflow for PC16.

> >>>>>

> >>>>> I think as per above.  An value within [-0xffff,0xffff]

> >>>>> (mathematically, i.e. the 32 or 64 bit value of the computation

> >>>>> S+A-P) is defined as not overflowing.  The linker basically needs to

> >>>>> assume that the author knew what he was doing when using a PC16

> >>>>> offset and only needs to flag things that simply cannot be made to

> >>>>> work (like e.g. jumping 0x12345 bytes forward).

> >>>>

> >>>> While the ABI is free to state what it wants, such a wide range is

> >>>> not very useful when the relocation is used outside of 16-bit mode.

> >>>

> >>> Well, as it stands the relocation needs to support uses within and

> >>> outside 16-bit mode, and the linker can't differ between those, so

> >>> what's acceptable needs to be the union of all ranges.

> >>>

> >>>> The main need for the overflow detection is when it's out of range by

> >>>> just a little (e.g. because of some piece of code or data having

> >>>> grown just enough to bring it out of range).

> >>>

> >>> But out of _what_ range?  You are saying that [-0xffff,0xffff] is too

> >>> broad, what range do you want?  (I don't think you can get away with a

> >>> smaller range with PC16 in the x86-64 psABI, but for completeness)

> >>

> >> The true disp16 range - [-0x8000,+0x7fff].

> >

> > Okay, well, yeah, I meanwhile think we can't do that generally :-/.  We

> > could possibly with an option, and I think we could have that option on by

> > default to be disabled by code that needs to embed 16bit code into elf32

> > or elf64 binaries.  (That option would extend the accepted range to

> > [-0xffff,0xffff])

>

> That's one of the things I proposed to H.J. earlier on. He took the

> position that even such projects should continue to work "out-of-the-

> box" with the next binutils release. I disagree, but he's the

> maintainer ...

>

> >> As said before, for the purposes of 32- and 64-bit code (and data), this

> >> (or a new, proper reloc type) needs to match PC32 allowing

> >> [-0x80000000,+0x7fffffff]. And obviously this also ought to match PC8,

> >> allowing [-0x80,+0x7f].

> >

> > I think for PC8 we have an easier time: there's no code model that wraps

> > around at 256, so allowing only [-0x80,0x7f] now and requiring

> > sign-extension to preserve the value ought to not give us any headaches in

> > the wild.

> >

> >> Also note how PC8's current handling in bfd does _not_ fit 16-bit

> >> code (wrapping at the 64k boundary not being accounted for).

> >

> > I don't think we "correctly" do that with any code model or relocation,

> > right?  The linker doesn't know the context of relocations, and hence

> > can't know if it would need to account for wrap-around at 64k, at 4G or

> > at 1<<64.  Or, if we want to get into extremes consider the A20 gate and a

> > wraparound at 1M.  There's only so much we can model.

>

> You don't mention the A20 gate here, do you? ;-)

>

> > I mean you can embed 32bit code in elf64, and use an PC32 reloc to get

> > from 3G to 0G, if you consider the wrap-around.  When only strictly

> > accepting the signed32 range that would be impossible, and we have the

> > same problem as with PC16: should we, or should we not, flag such use, and

> > under which conditions?

>

> Right, I did consider this PC32 issues as well. Embedding smaller

> address size code into an object targeting a larger address size

> architecture is of course always problematic. Hence why I think the

> PC32 handling is okay as is, but the PC16 one would better match in

> how it gets treated.

>

> > In such cases we need to consider existing use in the wild and provide a

> > way to accept existing .o files.  If those cases are seldom then possibly

> > only under an option.

>

> Right - the tradeoff between sufficiently precise checks (with false

> positive errors) and sufficiently imprecise checks (with false

> negatives, i.e. missing diagnostics). That's generally something only

> a human can decide, and only on a project by project or even component

> by component basis.

>

> >>>> This is what my change was about, which - aiui - H.J. has meanwhile

> >>>> reverted.

> >>>

> >>> If your change flagged values within [0x8000,0xffff] or [-0xffff,-0x8001]

> >>> by default then I think the reversal was justified, those need to be

> >>> continued to be accepted, for better or worse.

> >>

> >> Interesting - I took your initial reply to H.J. to mean the opposite.

> >> Obviously a misunderstanding then ...

> >

> > No, I changed my mind in the meantime.  I would have liked to say the

> > obvious thing in the psABI and not consider wrap-around, but now think we

> > can't go the full way for that.  I still would like us to make PC8 and

> > PC16 acceptable, but the latter with a larger than obvious acceptable

> > range.

> >

> > As for a new relocaiton that does have the signed16 restriction, no matter

> > the code model, I remain doubtful: it can introduce more (compatibility)

> > problems than it solves.

>

> Indeed, there is this risk. I continue to be in favor of a new command

> line option, off by default, restoring original PC16 checking and at

> the same time making PC32 in a 64-bit container match. (It could be two

> separate options, but to me this feels like going too far.) As per what

> you say above, I'd be happy to leave PC8 as is.

>

> H.J. - can we get you to re-consider?


The default behavior should stay the same since we are using PC16 only
in 16-bit programs.   We can add a new option to change PC16 overflow
behavior so that it can be used in 32-bit and 64-bit programs.

-- 
H.J.

Patch

diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
index c68741af02c..cf7cd076b17 100644
--- a/bfd/elf32-i386.c
+++ b/bfd/elf32-i386.c
@@ -22,7 +22,6 @@ 
 #include "elf-vxworks.h"
 #include "dwarf2.h"
 #include "opcode/i386.h"
-#include "libiberty.h"
 
 /* 386 uses REL relocations instead of RELA.  */
 #define USE_REL	1
@@ -94,7 +93,7 @@  static reloc_howto_type elf_howto_table[]=
   HOWTO(R_386_16, 0, 1, 16, false, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_386_16",
 	true, 0xffff, 0xffff, false),
-  HOWTO(R_386_PC16, 0, 1, 16, true, 0, complain_overflow_signed,
+  HOWTO(R_386_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_386_PC16",
 	true, 0xffff, 0xffff, true),
   HOWTO(R_386_8, 0, 0, 8, false, 0, complain_overflow_bitfield,
@@ -176,14 +175,10 @@  static reloc_howto_type elf_howto_table[]=
 	 false,			/* partial_inplace */
 	 0,			/* src_mask */
 	 0,			/* dst_mask */
-	 false),		/* pcrel_offset */
+	 false)			/* pcrel_offset */
 
 #define R_386_vt (R_386_GNU_VTENTRY + 1 - R_386_vt_offset)
 
-/* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
-  HOWTO(R_386_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
-	bfd_elf_generic_reloc, "R_386_PC16",
-	true, 0xffff, 0xffff, true)
 };
 
 #define X86_PCREL_TYPE_P(TYPE) ((TYPE) == R_386_PC32)
@@ -374,7 +369,7 @@  elf_i386_reloc_name_lookup (bfd *abfd ATTRIBUTE_UNUSED,
 }
 
 static reloc_howto_type *
-elf_i386_rtype_to_howto (bfd *abfd, unsigned r_type)
+elf_i386_rtype_to_howto (unsigned r_type)
 {
   unsigned int indx;
 
@@ -389,11 +384,6 @@  elf_i386_rtype_to_howto (bfd *abfd, unsigned r_type)
   /* PR 17512: file: 0f67f69d.  */
   if (elf_howto_table [indx].type != r_type)
     return NULL;
-
-  /* Use complain_overflow_bitfield on R_386_PC16 for code16.  */
-  if (r_type == (unsigned int) R_386_PC16 && elf_x86_has_code16 (abfd))
-    indx = ARRAY_SIZE (elf_howto_table) - 1;
-
   return &elf_howto_table[indx];
 }
 
@@ -404,8 +394,7 @@  elf_i386_info_to_howto_rel (bfd *abfd,
 {
   unsigned int r_type = ELF32_R_TYPE (dst->r_info);
 
-  if ((cache_ptr->howto = elf_i386_rtype_to_howto (abfd, r_type))
-      == NULL)
+  if ((cache_ptr->howto = elf_i386_rtype_to_howto (r_type)) == NULL)
     {
       /* xgettext:c-format */
       _bfd_error_handler (_("%pB: unsupported relocation type %#x"),
@@ -1153,8 +1142,8 @@  elf_i386_tls_transition (struct bfd_link_info *info, bfd *abfd,
       reloc_howto_type *from, *to;
       const char *name;
 
-      from = elf_i386_rtype_to_howto (abfd, from_type);
-      to = elf_i386_rtype_to_howto (abfd, to_type);
+      from = elf_i386_rtype_to_howto (from_type);
+      to = elf_i386_rtype_to_howto (to_type);
 
       if (h)
 	name = h->root.root.string;
@@ -2085,7 +2074,7 @@  elf_i386_relocate_section (bfd *output_bfd,
 	  continue;
 	}
 
-      howto = elf_i386_rtype_to_howto (input_bfd, r_type);
+      howto = elf_i386_rtype_to_howto (r_type);
       if (howto == NULL)
 	return _bfd_unrecognized_reloc (input_bfd, input_section, r_type);
 
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index d420561c156..98fb88113c0 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -82,7 +82,7 @@  static reloc_howto_type x86_64_elf_howto_table[] =
 	false),
   HOWTO(R_X86_64_16, 0, 1, 16, false, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_X86_64_16", false, 0, 0xffff, false),
-  HOWTO(R_X86_64_PC16, 0, 1, 16, true, 0, complain_overflow_signed,
+  HOWTO(R_X86_64_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_X86_64_PC16", false, 0, 0xffff, true),
   HOWTO(R_X86_64_8, 0, 0, 8, false, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_X86_64_8", false, 0, 0xff, false),
@@ -187,10 +187,6 @@  static reloc_howto_type x86_64_elf_howto_table[] =
 	 _bfd_elf_rel_vtable_reloc_fn, "R_X86_64_GNU_VTENTRY", false, 0, 0,
 	 false),
 
-/* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
-  HOWTO(R_X86_64_PC16, 0, 1, 16, true, 0, complain_overflow_bitfield,
-	bfd_elf_generic_reloc, "R_X86_64_PC16", false, 0, 0xffff, true),
-
 /* Use complain_overflow_bitfield on R_X86_64_32 for x32.  */
   HOWTO(R_X86_64_32, 0, 2, 32, false, 0, complain_overflow_bitfield,
 	bfd_elf_generic_reloc, "R_X86_64_32", false, 0, 0xffffffff,
@@ -274,14 +270,6 @@  elf_x86_64_rtype_to_howto (bfd *abfd, unsigned r_type)
       else
 	i = ARRAY_SIZE (x86_64_elf_howto_table) - 1;
     }
-  else if (r_type == (unsigned int) R_X86_64_PC16)
-    {
-      /* Use complain_overflow_bitfield on R_X86_64_PC16 for code16.  */
-      if (elf_x86_has_code16 (abfd))
-	i = ARRAY_SIZE (x86_64_elf_howto_table) - 2;
-      else
-	i = r_type;
-    }
   else if (r_type < (unsigned int) R_X86_64_GNU_VTINHERIT
 	   || r_type >= (unsigned int) R_X86_64_max)
     {
diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
index 29dc7f04b4d..62d516aab8d 100644
--- a/bfd/elfxx-x86.c
+++ b/bfd/elfxx-x86.c
@@ -2353,7 +2353,6 @@  _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
       || (type >= GNU_PROPERTY_X86_UINT32_OR_AND_LO
 	  && type <= GNU_PROPERTY_X86_UINT32_OR_AND_HI))
     {
-      unsigned int number;
       if (datasz != 4)
 	{
 	  _bfd_error_handler
@@ -2362,13 +2361,7 @@  _bfd_x86_elf_parse_gnu_properties (bfd *abfd, unsigned int type,
 	  return property_corrupt;
 	}
       prop = _bfd_elf_get_property (abfd, type, datasz);
-      number = bfd_h_get_32 (abfd, ptr);
-      if ((abfd->flags
-	   & (DYNAMIC | BFD_LINKER_CREATED | BFD_PLUGIN)) == 0
-	  && type == GNU_PROPERTY_X86_FEATURE_2_USED
-	  && (number & GNU_PROPERTY_X86_FEATURE_2_CODE16) != 0)
-	elf_x86_has_code16 (abfd) = 1;
-      prop->u.number |= number;
+      prop->u.number |= bfd_h_get_32 (abfd, ptr);
       prop->pr_kind = property_number;
       return property_number;
     }
diff --git a/bfd/elfxx-x86.h b/bfd/elfxx-x86.h
index e8344305492..db11327e96f 100644
--- a/bfd/elfxx-x86.h
+++ b/bfd/elfxx-x86.h
@@ -549,9 +549,6 @@  struct elf_x86_obj_tdata
 
   /* GOTPLT entries for TLS descriptors.  */
   bfd_vma *local_tlsdesc_gotent;
-
-  /* Set if the objec file has 16-bit code.  */
-  unsigned int has_code16 : 1;
 };
 
 enum elf_x86_plt_type
@@ -587,9 +584,6 @@  struct elf_x86_plt
 #define elf_x86_local_tlsdesc_gotent(abfd) \
   (elf_x86_tdata (abfd)->local_tlsdesc_gotent)
 
-#define elf_x86_has_code16(abfd) \
-  (elf_x86_tdata (abfd)->has_code16)
-
 #define elf_x86_compute_jump_table_size(htab) \
   ((htab)->elf.srelplt->reloc_count * (htab)->got_entry_size)
 
diff --git a/binutils/readelf.c b/binutils/readelf.c
index a6ed24c03bd..d773b9a4931 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -19139,9 +19139,6 @@  decode_x86_feature_2 (unsigned int bitmask)
 	case GNU_PROPERTY_X86_FEATURE_2_XSAVEC:
 	  printf ("XSAVEC");
 	  break;
-	case GNU_PROPERTY_X86_FEATURE_2_CODE16:
-	  printf ("CODE16");
-	  break;
 	default:
 	  printf (_("<unknown: %x>"), bit);
 	  break;
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index c17f4da16fe..d3441988e34 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2695,10 +2695,6 @@  static void
 set_code_flag (int value)
 {
   update_code_flag (value, 0);
-#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
-  if (value == CODE_16BIT)
-    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
-#endif
 }
 
 static void
@@ -2710,10 +2706,6 @@  set_16bit_gcc_code_flag (int new_code_flag)
   cpu_arch_flags.bitfield.cpu64 = 0;
   cpu_arch_flags.bitfield.cpuno64 = 1;
   stackop_size = LONG_MNEM_SUFFIX;
-#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
-  if (new_code_flag == CODE_16BIT)
-    x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_CODE16;
-#endif
 }
 
 static void
@@ -9040,7 +9032,7 @@  x86_cleanup (void)
   unsigned int isa_1_descsz_raw, feature_2_descsz_raw;
   unsigned int padding;
 
-  if (!IS_ELF || (!x86_used_note && !x86_feature_2_used))
+  if (!IS_ELF || !x86_used_note)
     return;
 
   x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X86;
@@ -9080,23 +9072,15 @@  x86_cleanup (void)
   bfd_set_section_alignment (sec, alignment);
   elf_section_type (sec) = SHT_NOTE;
 
-  if (x86_used_note)
-    {
-      /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
-	 + 4-byte data  */
-      isa_1_descsz_raw = 4 + 4 + 4;
-      /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
-      isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
-    }
-  else
-    {
-      isa_1_descsz_raw = 0;
-      isa_1_descsz = 0;
-    }
+  /* GNU_PROPERTY_X86_ISA_1_USED: 4-byte type + 4-byte data size
+				  + 4-byte data  */
+  isa_1_descsz_raw = 4 + 4 + 4;
+  /* Align GNU_PROPERTY_X86_ISA_1_USED.  */
+  isa_1_descsz = (isa_1_descsz_raw + align_size_1) & ~align_size_1;
 
   feature_2_descsz_raw = isa_1_descsz;
   /* GNU_PROPERTY_X86_FEATURE_2_USED: 4-byte type + 4-byte data size
-     + 4-byte data  */
+				      + 4-byte data  */
   feature_2_descsz_raw += 4 + 4 + 4;
   /* Align GNU_PROPERTY_X86_FEATURE_2_USED.  */
   feature_2_descsz = ((feature_2_descsz_raw + align_size_1)
@@ -9118,23 +9102,20 @@  x86_cleanup (void)
   /* Write n_name.  */
   memcpy (p + 4 * 3, "GNU", 4);
 
-  if (isa_1_descsz != 0)
-    {
-      /* Write 4-byte type.  */
-      md_number_to_chars (p + 4 * 4,
-			  (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
+  /* Write 4-byte type.  */
+  md_number_to_chars (p + 4 * 4,
+		      (valueT) GNU_PROPERTY_X86_ISA_1_USED, 4);
 
-      /* Write 4-byte data size.  */
-      md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
+  /* Write 4-byte data size.  */
+  md_number_to_chars (p + 4 * 5, (valueT) 4, 4);
 
-      /* Write 4-byte data.  */
-      md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
+  /* Write 4-byte data.  */
+  md_number_to_chars (p + 4 * 6, (valueT) x86_isa_1_used, 4);
 
-      /* Zero out paddings.  */
-      padding = isa_1_descsz - isa_1_descsz_raw;
-      if (padding)
-	memset (p + 4 * 7, 0, padding);
-    }
+  /* Zero out paddings.  */
+  padding = isa_1_descsz - isa_1_descsz_raw;
+  if (padding)
+    memset (p + 4 * 7, 0, padding);
 
   /* Write 4-byte type.  */
   md_number_to_chars (p + isa_1_descsz + 4 * 4,
diff --git a/gas/testsuite/gas/i386/code16-2.d b/gas/testsuite/gas/i386/code16-2.d
index 37b66c85f4e..f18c8cd62da 100644
--- a/gas/testsuite/gas/i386/code16-2.d
+++ b/gas/testsuite/gas/i386/code16-2.d
@@ -1,8 +1,3 @@ 
 #name: i386 code16 2
 #as: -mx86-used-note=no --generate-missing-build-notes=no
 #readelf: -n
-
-Displaying notes found in: .note.gnu.property
-[ 	]+Owner[ 	]+Data size[ 	]+Description
-  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
-      Properties: x86 feature used: x86, CODE16
diff --git a/gas/testsuite/gas/i386/x86-64-code16-2.d b/gas/testsuite/gas/i386/x86-64-code16-2.d
index dbabd67e888..5052353c00d 100644
--- a/gas/testsuite/gas/i386/x86-64-code16-2.d
+++ b/gas/testsuite/gas/i386/x86-64-code16-2.d
@@ -2,8 +2,3 @@ 
 #name: x86-64 code16 2
 #as: -mx86-used-note=no --generate-missing-build-notes=no
 #readelf: -n
-
-Displaying notes found in: .note.gnu.property
-[ 	]+Owner[ 	]+Data size[ 	]+Description
-  GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
-      Properties: x86 feature used: x86, CODE16
diff --git a/include/elf/common.h b/include/elf/common.h
index 564ab711a20..24d0a09b7c8 100644
--- a/include/elf/common.h
+++ b/include/elf/common.h
@@ -872,7 +872,6 @@ 
 #define GNU_PROPERTY_X86_FEATURE_2_XSAVEC	(1U << 9)
 #define GNU_PROPERTY_X86_FEATURE_2_TMM		(1U << 10)
 #define GNU_PROPERTY_X86_FEATURE_2_MASK		(1U << 11)
-#define GNU_PROPERTY_X86_FEATURE_2_CODE16	(1U << 12)
 
 #define GNU_PROPERTY_X86_COMPAT_2_ISA_1_NEEDED \
   (GNU_PROPERTY_X86_UINT32_OR_LO + 0)
diff --git a/ld/testsuite/ld-i386/pcrel16-2.d b/ld/testsuite/ld-i386/pcrel16-2.d
index c1c340fc75c..de9c779e810 100644
--- a/ld/testsuite/ld-i386/pcrel16-2.d
+++ b/ld/testsuite/ld-i386/pcrel16-2.d
@@ -3,3 +3,4 @@ 
 #ld: -melf_i386
 #error: .*relocation truncated to fit: R_386_PC16 .*t16.*
 #error: .*relocation truncated to fit: R_386_PC16 .*_start.*
+#xfail: *-*-*
diff --git a/ld/testsuite/ld-x86-64/pcrel16-2.d b/ld/testsuite/ld-x86-64/pcrel16-2.d
index 5346a5b619d..991dcd09646 100644
--- a/ld/testsuite/ld-x86-64/pcrel16-2.d
+++ b/ld/testsuite/ld-x86-64/pcrel16-2.d
@@ -3,3 +3,4 @@ 
 #ld:
 #error: .*relocation truncated to fit: R_X86_64_PC16 .*t16.*
 #error: .*relocation truncated to fit: R_X86_64_PC16 .*_start.*
+#xfail: *-*-*