Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Jinyang He]

On /Thu Aug 10 07:55:58 GMT 2023 /Jan Beulich wrote:

> On 10.08.2023 04:21, Jinyang He wrote:
> >/The DW_CFA_advance_loc4 may be in different frags. Then fr_fix />/may caused something wrong. Referenced by commit b9d8f5601bcf />/("Re: Optimise away eh_frame advance_loc 0"). /
> I'm afraid I don't understand that earlier fix: frag_more(1) there
> ought to guarantee fr_fix (once the frag is closed) to be >= 1.
> It would then seem to me that ...

I'm not familiar with it. Please point out my mistakes. Thanks in advance.

DW_CFA_advance_loc4 requires 1 byte, and follows contents requires
no more than 4 bytes. In state_saw_loc4-case, it can be optimized
to DW_CFA_advance_{loc, loc1, loc2, loc4}. Current the two processes
are asynchronous. It means that the state_wait_loc4-case exit
check_eh_frame() firstly and then enter state_saw_loc4-case when
reenter check_eh_frame(). Thus, we actually need these 5 bytes to be
in the same frags and the loc4_fix point the first char.


>
> >/--- a/gas/ehopt.c />/+++ b/gas/ehopt.c />/@@ -386,7 +386,7 @@ check_eh_frame (expressionS *exp, unsigned int 
> *pnbytes) />/{ />//* This might be a DW_CFA_advance_loc4. Record the frag and the />/position within the frag, so that we can change it later. */ />/- frag_grow (1); />/+ frag_grow (1 + 4); />/d->state = state_saw_loc4; />/d->loc4_frag = frag_now; />/d->loc4_fix = frag_now_fix (); /
> ... here instead we also need
>
> 	  frag_more (1);
Either frag_more or frag_grow does not actually change frag_now
if frchain_now has enough rooms (rooms >= nchar). Compare to frag_grow,
frag_more has return values and empty space to zeros. Both frag_more
or frag_grow, they should check 5 bytes. Thus, simply call "frag_grow(5)"

here to ensure that both parts of DW_CFA_advance_* are in the same frags.


> 	  d->state = state_saw_loc4;
> 	  d->loc4_frag = frag_now;
> 	  d->loc4_fix = frag_now_fix () - 1;
>
> Otoh if frags were always grown (there and here), couldn't we do away
> with loc4_frag and loc4_fix?
>
> As to your 2.41 question: You realize the release was already cut?
> Putting this (or whichever else) fix there would be likely be okay, but
> would have a real effect only if 2.41.1 was ever made.
I know it's been released. I just wish it was available in [1].

[1] 
https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/binutils-2_41-branch


Jinyang

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Jan Beulich]

On 10.08.2023 14:36, Jinyang He wrote:
> On /Thu Aug 10 07:55:58 GMT 2023 /Jan Beulich wrote:
> 
>> On 10.08.2023 04:21, Jinyang He wrote:
>>> /The DW_CFA_advance_loc4 may be in different frags. Then fr_fix />/may caused something wrong. Referenced by commit b9d8f5601bcf />/("Re: Optimise away eh_frame advance_loc 0"). /
>> I'm afraid I don't understand that earlier fix: frag_more(1) there
>> ought to guarantee fr_fix (once the frag is closed) to be >= 1.
>> It would then seem to me that ...
> 
> I'm not familiar with it. Please point out my mistakes. Thanks in advance.

Well, Alan has approved your change. Maybe it's me who is wrong here.

Jan

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Alan Modra]

On Thu, Aug 10, 2023 at 02:55:20PM +0200, Jan Beulich wrote:
> On 10.08.2023 14:36, Jinyang He wrote:
> > On /Thu Aug 10 07:55:58 GMT 2023 /Jan Beulich wrote:
> > 
> >> On 10.08.2023 04:21, Jinyang He wrote:
> >>> /The DW_CFA_advance_loc4 may be in different frags. Then fr_fix />/may caused something wrong. Referenced by commit b9d8f5601bcf />/("Re: Optimise away eh_frame advance_loc 0"). /
> >> I'm afraid I don't understand that earlier fix: frag_more(1) there
> >> ought to guarantee fr_fix (once the frag is closed) to be >= 1.
> >> It would then seem to me that ...
> > 
> > I'm not familiar with it. Please point out my mistakes. Thanks in advance.
> 
> Well, Alan has approved your change. Maybe it's me who is wrong here.

The idea behind commit b9d8f5601bcf was that when a
DW_CFA_advance_loc4 of zero is seen in eh_frame_relax_frag and
eh_frame_convert_frag we want to remove the opcode entirely, not just
convert to a nop.  If the opcode was split over two frags then the
size adjustment would need to be done to the first frag, not the
second as is correct for other cases with split frags.  This would
complicate the eh relaxation.  It's easier to ensure the frag is not
split.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Jan Beulich]

On 11.08.2023 01:02, Alan Modra wrote:
> On Thu, Aug 10, 2023 at 02:55:20PM +0200, Jan Beulich wrote:
>> On 10.08.2023 14:36, Jinyang He wrote:
>>> On /Thu Aug 10 07:55:58 GMT 2023 /Jan Beulich wrote:
>>>
>>>> On 10.08.2023 04:21, Jinyang He wrote:
>>>>> /The DW_CFA_advance_loc4 may be in different frags. Then fr_fix />/may caused something wrong. Referenced by commit b9d8f5601bcf />/("Re: Optimise away eh_frame advance_loc 0"). /
>>>> I'm afraid I don't understand that earlier fix: frag_more(1) there
>>>> ought to guarantee fr_fix (once the frag is closed) to be >= 1.
>>>> It would then seem to me that ...
>>>
>>> I'm not familiar with it. Please point out my mistakes. Thanks in advance.
>>
>> Well, Alan has approved your change. Maybe it's me who is wrong here.
> 
> The idea behind commit b9d8f5601bcf was that when a
> DW_CFA_advance_loc4 of zero is seen in eh_frame_relax_frag and
> eh_frame_convert_frag we want to remove the opcode entirely, not just
> convert to a nop.  If the opcode was split over two frags then the
> size adjustment would need to be done to the first frag, not the
> second as is correct for other cases with split frags.  This would
> complicate the eh relaxation.  It's easier to ensure the frag is not
> split.

Oh, right, I can see that this makes things easier. But for the change
here, does frag_grow() actually help preventing the split? Wouldn't it
need to be yet beyond what I suggested and require frag_more(5), such
that intermediate section switches wouldn't close off the first frag?
Of course this would collide with what you say regarding size
adjustments needing to happen on the 2nd (variable) frag. (Aiui just
frag_grow() is enough in output_cfi_insn(), as there we aren't at risk
of intermediate section changes.)

Jan

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Alan Modra]

On Fri, Aug 11, 2023 at 10:27:53AM +0200, Jan Beulich wrote:
> On 11.08.2023 01:02, Alan Modra wrote:
> > On Thu, Aug 10, 2023 at 02:55:20PM +0200, Jan Beulich wrote:
> >> On 10.08.2023 14:36, Jinyang He wrote:
> >>> On /Thu Aug 10 07:55:58 GMT 2023 /Jan Beulich wrote:
> >>>
> >>>> On 10.08.2023 04:21, Jinyang He wrote:
> >>>>> /The DW_CFA_advance_loc4 may be in different frags. Then fr_fix />/may caused something wrong. Referenced by commit b9d8f5601bcf />/("Re: Optimise away eh_frame advance_loc 0"). /
> >>>> I'm afraid I don't understand that earlier fix: frag_more(1) there
> >>>> ought to guarantee fr_fix (once the frag is closed) to be >= 1.
> >>>> It would then seem to me that ...
> >>>
> >>> I'm not familiar with it. Please point out my mistakes. Thanks in advance.
> >>
> >> Well, Alan has approved your change. Maybe it's me who is wrong here.
> > 
> > The idea behind commit b9d8f5601bcf was that when a
> > DW_CFA_advance_loc4 of zero is seen in eh_frame_relax_frag and
> > eh_frame_convert_frag we want to remove the opcode entirely, not just
> > convert to a nop.  If the opcode was split over two frags then the
> > size adjustment would need to be done to the first frag, not the
> > second as is correct for other cases with split frags.  This would
> > complicate the eh relaxation.  It's easier to ensure the frag is not
> > split.
> 
> Oh, right, I can see that this makes things easier. But for the change
> here, does frag_grow() actually help preventing the split?

Yes.

> Wouldn't it
> need to be yet beyond what I suggested and require frag_more(5),

No, because frag_more would move the obstack data pointer, and we want
to leave that to the caller of check_eh_frame.

> such
> that intermediate section switches wouldn't close off the first frag?

That shouldn't happen.  check_eh_frame only does something when
processing .eh_frame or .debug_frame sections.

> Of course this would collide with what you say regarding size
> adjustments needing to happen on the 2nd (variable) frag. (Aiui just
> frag_grow() is enough in output_cfi_insn(), as there we aren't at risk
> of intermediate section changes.)
> 
> Jan

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Jinyang He]

Hi,

How about this patch state?


Jinyang


On 2023-08-11 18:15, Alan Modra wrote:
> On Fri, Aug 11, 2023 at 10:27:53AM +0200, Jan Beulich wrote:
>> On 11.08.2023 01:02, Alan Modra wrote:
>>> On Thu, Aug 10, 2023 at 02:55:20PM +0200, Jan Beulich wrote:
>>>> On 10.08.2023 14:36, Jinyang He wrote:
>>>>> On /Thu Aug 10 07:55:58 GMT 2023 /Jan Beulich wrote:
>>>>>
>>>>>> On 10.08.2023 04:21, Jinyang He wrote:
>>>>>>> /The DW_CFA_advance_loc4 may be in different frags. Then fr_fix />/may caused something wrong. Referenced by commit b9d8f5601bcf />/("Re: Optimise away eh_frame advance_loc 0"). /
>>>>>> I'm afraid I don't understand that earlier fix: frag_more(1) there
>>>>>> ought to guarantee fr_fix (once the frag is closed) to be >= 1.
>>>>>> It would then seem to me that ...
>>>>> I'm not familiar with it. Please point out my mistakes. Thanks in advance.
>>>> Well, Alan has approved your change. Maybe it's me who is wrong here.
>>> The idea behind commit b9d8f5601bcf was that when a
>>> DW_CFA_advance_loc4 of zero is seen in eh_frame_relax_frag and
>>> eh_frame_convert_frag we want to remove the opcode entirely, not just
>>> convert to a nop.  If the opcode was split over two frags then the
>>> size adjustment would need to be done to the first frag, not the
>>> second as is correct for other cases with split frags.  This would
>>> complicate the eh relaxation.  It's easier to ensure the frag is not
>>> split.
>> Oh, right, I can see that this makes things easier. But for the change
>> here, does frag_grow() actually help preventing the split?
> Yes.
>
>> Wouldn't it
>> need to be yet beyond what I suggested and require frag_more(5),
> No, because frag_more would move the obstack data pointer, and we want
> to leave that to the caller of check_eh_frame.
>
>> such
>> that intermediate section switches wouldn't close off the first frag?
> That shouldn't happen.  check_eh_frame only does something when
> processing .eh_frame or .debug_frame sections.
>
>> Of course this would collide with what you say regarding size
>> adjustments needing to happen on the 2nd (variable) frag. (Aiui just
>> frag_grow() is enough in output_cfi_insn(), as there we aren't at risk
>> of intermediate section changes.)
>>
>> Jan

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Jan Beulich]

On 30.08.2023 05:35, Jinyang He wrote:
> How about this patch state?

As said before, Alan approved your change, so you're okay to commit. In
the subsequent discussion he explained to me why what I was suggesting
wasn't really correct.

Jan

> On 2023-08-11 18:15, Alan Modra wrote:
>> On Fri, Aug 11, 2023 at 10:27:53AM +0200, Jan Beulich wrote:
>>> On 11.08.2023 01:02, Alan Modra wrote:
>>>> On Thu, Aug 10, 2023 at 02:55:20PM +0200, Jan Beulich wrote:
>>>>> On 10.08.2023 14:36, Jinyang He wrote:
>>>>>> On /Thu Aug 10 07:55:58 GMT 2023 /Jan Beulich wrote:
>>>>>>
>>>>>>> On 10.08.2023 04:21, Jinyang He wrote:
>>>>>>>> /The DW_CFA_advance_loc4 may be in different frags. Then fr_fix />/may caused something wrong. Referenced by commit b9d8f5601bcf />/("Re: Optimise away eh_frame advance_loc 0"). /
>>>>>>> I'm afraid I don't understand that earlier fix: frag_more(1) there
>>>>>>> ought to guarantee fr_fix (once the frag is closed) to be >= 1.
>>>>>>> It would then seem to me that ...
>>>>>> I'm not familiar with it. Please point out my mistakes. Thanks in advance.
>>>>> Well, Alan has approved your change. Maybe it's me who is wrong here.
>>>> The idea behind commit b9d8f5601bcf was that when a
>>>> DW_CFA_advance_loc4 of zero is seen in eh_frame_relax_frag and
>>>> eh_frame_convert_frag we want to remove the opcode entirely, not just
>>>> convert to a nop.  If the opcode was split over two frags then the
>>>> size adjustment would need to be done to the first frag, not the
>>>> second as is correct for other cases with split frags.  This would
>>>> complicate the eh relaxation.  It's easier to ensure the frag is not
>>>> split.
>>> Oh, right, I can see that this makes things easier. But for the change
>>> here, does frag_grow() actually help preventing the split?
>> Yes.
>>
>>> Wouldn't it
>>> need to be yet beyond what I suggested and require frag_more(5),
>> No, because frag_more would move the obstack data pointer, and we want
>> to leave that to the caller of check_eh_frame.
>>
>>> such
>>> that intermediate section switches wouldn't close off the first frag?
>> That shouldn't happen.  check_eh_frame only does something when
>> processing .eh_frame or .debug_frame sections.
>>
>>> Of course this would collide with what you say regarding size
>>> adjustments needing to happen on the 2nd (variable) frag. (Aiui just
>>> frag_grow() is enough in output_cfi_insn(), as there we aren't at risk
>>> of intermediate section changes.)
>>>
>>> Jan
> 

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Alan Modra]

On Wed, Aug 30, 2023 at 08:28:56AM +0200, Jan Beulich wrote:
> On 30.08.2023 05:35, Jinyang He wrote:
> > How about this patch state?
> 
> As said before, Alan approved your change, so you're okay to commit. In
> the subsequent discussion he explained to me why what I was suggesting
> wasn't really correct.

I pushed the patch, with the explanation I gave regarding commit
b9d8f5601bcf in the log (cleaned up a little to be more accurate).

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Jinyang He]

The DW_CFA_advance_loc4 may be in different frags. Then fr_fix
may caused something wrong. Referenced by commit b9d8f5601bcf
("Re: Optimise away eh_frame advance_loc 0").

gas/ChangeLog:

	* ehopt.c (check_eh_frame): Don't allow DW_CFA_advance_loc4
	to be placed in a different frag to the rs_cfa.
---
BTW, it is a bug I triggered when compiling a LoongArch.asm file with
relaxation being enabled. I hope it could be merged to 2.41 as bug-fix.
 
 gas/ehopt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gas/ehopt.c b/gas/ehopt.c
index feea61b92..9d6606adf 100644
--- a/gas/ehopt.c
+++ b/gas/ehopt.c
@@ -386,7 +386,7 @@ check_eh_frame (expressionS *exp, unsigned int *pnbytes)
 	{
 	  /* This might be a DW_CFA_advance_loc4.  Record the frag and the
 	     position within the frag, so that we can change it later.  */
-	  frag_grow (1);
+	  frag_grow (1 + 4);
 	  d->state = state_saw_loc4;
 	  d->loc4_frag = frag_now;
 	  d->loc4_fix = frag_now_fix ();
-- 
2.34.3

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Alan Modra]

On Thu, Aug 10, 2023 at 10:21:40AM +0800, Jinyang He wrote:
> The DW_CFA_advance_loc4 may be in different frags. Then fr_fix
> may caused something wrong. Referenced by commit b9d8f5601bcf
> ("Re: Optimise away eh_frame advance_loc 0").
> 
> gas/ChangeLog:
> 
> 	* ehopt.c (check_eh_frame): Don't allow DW_CFA_advance_loc4
> 	to be placed in a different frag to the rs_cfa.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Make sure DW_CFA_advance_loc4 is in the same frag

[Jan Beulich]

On 10.08.2023 04:21, Jinyang He wrote:
> The DW_CFA_advance_loc4 may be in different frags. Then fr_fix
> may caused something wrong. Referenced by commit b9d8f5601bcf
> ("Re: Optimise away eh_frame advance_loc 0").

I'm afraid I don't understand that earlier fix: frag_more(1) there
ought to guarantee fr_fix (once the frag is closed) to be >= 1.
It would then seem to me that ...

> --- a/gas/ehopt.c
> +++ b/gas/ehopt.c
> @@ -386,7 +386,7 @@ check_eh_frame (expressionS *exp, unsigned int *pnbytes)
>  	{
>  	  /* This might be a DW_CFA_advance_loc4.  Record the frag and the
>  	     position within the frag, so that we can change it later.  */
> -	  frag_grow (1);
> +	  frag_grow (1 + 4);
>  	  d->state = state_saw_loc4;
>  	  d->loc4_frag = frag_now;
>  	  d->loc4_fix = frag_now_fix ();

... here instead we also need

	  frag_more (1);
	  d->state = state_saw_loc4;
	  d->loc4_frag = frag_now;
	  d->loc4_fix = frag_now_fix () - 1;

Otoh if frags were always grown (there and here), couldn't we do away
with loc4_frag and loc4_fix?

As to your 2.41 question: You realize the release was already cut?
Putting this (or whichever else) fix there would be likely be okay, but
would have a real effect only if 2.41.1 was ever made.

Jan