Hiello Pedro,

>>  > - I think it's more clear to only set lr_register when needed (pc
>>  > reaches the limit), as opposed to resetting it to -1 if pc didn't reach
>>  > the limit.
>>
>> The body of condition that becomes visitable after this patch 
>> invalidates lr_reg by setting it to -2 before reaching the limit.
> 
> Sorry, I was referring to to fdata->lr_register. What I meant is that it
> seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end
> of the function (as Kevin had suggested), as opposed to setting it to -1
> at the end of the function, in this part:
> 

I didn't explain it right. The catch is that fdata->lr_register is set
by shifting lr_reg variable which is set only once and it can be
invalidated druing the loop iteration. For example, lets say that
firstly lr_reg is set to some value, then in some further loop iteration
the next code gets visited:

          else if (lr_reg >= 0 &&
             /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
             (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
        /* stw Rx, NUM(r1) */
        ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
        /* stwu Rx, NUM(r1) */
        ((op & 0xffff0000) == (lr_reg | 0x94010000))))
      { /* where Rx == lr */
        fdata->lr_offset = offset;
        fdata->nosavedpc = 0;
        /* Invalidate lr_reg, but don't set it to -1.
           That would mean that it had never been set.  */
        lr_reg = -2;

This part of code was previously never visited because of lr_reg
shifting. The last line in upper code overwrites value of lr_reg. If in
the furtherer iteration lim_pc would be reached, -2 value would be
shifted and set to fdata->lr_register and that is not what we want.


>
> Previously if there was a mflr/stw sequence that invalidated lr_reg,
> fdata->lr_register would be -1 when the function exited, even if pc ==
> lim_pc (because of the second term of the condition).
>


Previously if there was s mflr/stw sequence lr_reg would never be
invalidated (set to -2) because part of the code that does this
invalidation was never visitable due to shifting of lr_reg. Invalidation
is performed so that the upper code never gets visited again in same
iteration. lr_reg is set only once and it's value is used only in the
condition of upper part of the code. In other conditions it is only
checked whether it is set (different than -1). The comment where lr_reg
is changed from -1 to some value says the following:

"remember just the first one, but skip over additional ones."

Which means that we need to deal only with one value of lr_reg. And the
fdata->lr_register basically saves the register number that we get by
shifting lr_reg.

> 
> In any case, for this part to be correct, shouldn't the condition be the
> full complement of the original condition?
> 
> That is,
> 
> if (pc != lim_pc || lr_reg < 0)
> 
> Which is the complement of:
> 
> if (pc == lim_pc && lr_reg >= 0)
> 
> Instead of:
> 
>> +  if (pc != lim_pc)


I've changed this condition to following:

   if (pc != lim_pc && lr_reg != -1)

This is more appropriate condition because this condition requires
change of lr_reg.


> Still, I think it's safer to keep fdata->lr_register == -1 and set it at
> the end if needed.
> 


This could be achieved by using one more variable that would be backup
for lr_reg in case it is set to -2. But I think that this approach with
setting it to -1 is more appropriate than that?

Thanks,

Nikola Prica