[COMMITTED] libsframe: revisit sframe_find_fre API
Checks
Commit Message
Inspite of implementing a rather simple functionality, this function was
relatively difficult to follow, and maintain. Some changes are done now
to address that - refactor the function and use better names to make it
more readable.
The changes to the implementation do not cause any change in the
contract of the API.
libsframe/
* sframe.c (sframe_fre_get_end_ip_offset): to here...
(sframe_find_fre): Refactor some bits from...
---
libsframe/sframe.c | 83 +++++++++++++++++++++++++++-------------------
1 file changed, 49 insertions(+), 34 deletions(-)
Comments
On 26.05.2023 09:01, Indu Bhagat via Binutils wrote:
> @@ -1022,40 +1049,28 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
> bitmask = 0xff;
>
> fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
> + func_start_addr = fdep->sfde_func_start_address;
> +
> for (i = 0; i < fdep->sfde_func_num_fres; i++)
> {
> - err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
> - start_address = next_fre.fre_start_addr;
> + err = sframe_decode_fre (fres, &cur_fre, fre_type, &size);
> + if (err)
> + return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>
> - if (((fdep->sfde_func_start_address
> - + (int32_t) start_address) & bitmask) <= (pc & bitmask))
> + start_ip = func_start_addr + cur_fre.fre_start_addr;
> + end_ip_offset = sframe_fre_get_end_ip_offset (fdep, i, fres + size);
> + end_ip = func_start_addr + end_ip_offset;
> +
> + if ((start_ip & bitmask) > (pc & bitmask))
> + return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
> +
> + if (((start_ip & bitmask) <= (pc & bitmask))
> + && (end_ip & bitmask) >= (pc & bitmask))
> {
> - sframe_frame_row_entry_copy (&cur_fre, &next_fre);
> -
> - /* Get the next FRE in sequence. */
> - if (i < fdep->sfde_func_num_fres - 1)
> - {
> - sp += esz;
The buildbot failure just reported was found on the commit prior to this
one, which removed the declaration of sp. Please try to make sure that
every patch builds (and works) okay individually.
Jan
On 5/26/23 00:30, Jan Beulich wrote:
> On 26.05.2023 09:01, Indu Bhagat via Binutils wrote:
>> @@ -1022,40 +1049,28 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
>> bitmask = 0xff;
>>
>> fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
>> + func_start_addr = fdep->sfde_func_start_address;
>> +
>> for (i = 0; i < fdep->sfde_func_num_fres; i++)
>> {
>> - err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
>> - start_address = next_fre.fre_start_addr;
>> + err = sframe_decode_fre (fres, &cur_fre, fre_type, &size);
>> + if (err)
>> + return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>>
>> - if (((fdep->sfde_func_start_address
>> - + (int32_t) start_address) & bitmask) <= (pc & bitmask))
>> + start_ip = func_start_addr + cur_fre.fre_start_addr;
>> + end_ip_offset = sframe_fre_get_end_ip_offset (fdep, i, fres + size);
>> + end_ip = func_start_addr + end_ip_offset;
>> +
>> + if ((start_ip & bitmask) > (pc & bitmask))
>> + return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>> +
>> + if (((start_ip & bitmask) <= (pc & bitmask))
>> + && (end_ip & bitmask) >= (pc & bitmask))
>> {
>> - sframe_frame_row_entry_copy (&cur_fre, &next_fre);
>> -
>> - /* Get the next FRE in sequence. */
>> - if (i < fdep->sfde_func_num_fres - 1)
>> - {
>> - sp += esz;
>
> The buildbot failure just reported was found on the commit prior to this
> one, which removed the declaration of sp. Please try to make sure that
> every patch builds (and works) okay individually.
>
> Jan
[(Re)Sending to binutils@sourceware, I earlier sent my apologies to the
builder@ ;-) ]
Yes, my apologies. Turns out the 4 commits I just did were not
bisectable truly. I split a change into two logical commits, and missed
testing them one by one.
Revision: 812d868850126d8e791795c7e248ffbf580925f6 causes build
failures, but the subsequent commit (Revision:
83c219872b2131546ccec7edc57eb47c64b8911d) fixes the issue.
Will take care in future.
Indu
@@ -980,6 +980,32 @@ sframe_get_funcdesc_with_addr (sframe_decoder_ctx *ctx,
return sframe_ret_set_errno (errp, SFRAME_ERR_FDE_NOTFOUND);
}
+/* Get the end IP offset for the FRE at index i in the FDEP. The buffer FRES
+ is the starting location for the FRE. */
+
+static uint32_t
+sframe_fre_get_end_ip_offset (sframe_func_desc_entry *fdep, unsigned int i,
+ const char *fres)
+{
+ uint32_t end_ip_offset;
+ uint32_t fre_type;
+
+ fre_type = sframe_get_fre_type (fdep);
+
+ /* Get the start address of the next FRE in sequence. */
+ if (i < fdep->sfde_func_num_fres - 1)
+ {
+ sframe_decode_fre_start_address (fres, &end_ip_offset, fre_type);
+ end_ip_offset -= 1;
+ }
+ else
+ /* The end IP offset for the FRE needs to be deduced from the function
+ size. */
+ end_ip_offset = fdep->sfde_func_size - 1;
+
+ return end_ip_offset;
+}
+
/* Find the SFrame Row Entry which contains the PC. Returns
SFRAME_ERR if failure. */
@@ -987,14 +1013,15 @@ int
sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
sframe_frame_row_entry *frep)
{
+ sframe_frame_row_entry cur_fre;
sframe_func_desc_entry *fdep;
- uint32_t start_address, i;
- sframe_frame_row_entry cur_fre, next_fre;
- const char *fres;
unsigned int fre_type, fde_type;
- size_t esz;
- int err = 0;
+ uint32_t end_ip_offset, i;
+ int32_t start_ip, end_ip;
+ int32_t func_start_addr;
+ const char *fres;
size_t size = 0;
+ int err = 0;
/* For regular FDEs (i.e. fde_type SFRAME_FDE_TYPE_PCINC),
where the start address in the FRE is an offset from start pc,
use a bitmask with all bits set so that none of the address bits are
@@ -1022,40 +1049,28 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
bitmask = 0xff;
fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
+ func_start_addr = fdep->sfde_func_start_address;
+
for (i = 0; i < fdep->sfde_func_num_fres; i++)
{
- err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
- start_address = next_fre.fre_start_addr;
+ err = sframe_decode_fre (fres, &cur_fre, fre_type, &size);
+ if (err)
+ return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
- if (((fdep->sfde_func_start_address
- + (int32_t) start_address) & bitmask) <= (pc & bitmask))
+ start_ip = func_start_addr + cur_fre.fre_start_addr;
+ end_ip_offset = sframe_fre_get_end_ip_offset (fdep, i, fres + size);
+ end_ip = func_start_addr + end_ip_offset;
+
+ if ((start_ip & bitmask) > (pc & bitmask))
+ return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
+
+ if (((start_ip & bitmask) <= (pc & bitmask))
+ && (end_ip & bitmask) >= (pc & bitmask))
{
- sframe_frame_row_entry_copy (&cur_fre, &next_fre);
-
- /* Get the next FRE in sequence. */
- if (i < fdep->sfde_func_num_fres - 1)
- {
- sp += esz;
- err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
-
- /* Sanity check the next FRE. */
- if (!sframe_fre_sanity_check_p (&next_fre))
- return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
-
- size = next_fre.fre_start_addr;
- }
- else size = fdep->sfde_func_size;
-
- /* If the cur FRE is the one that contains the PC, return it. */
- if (((fdep->sfde_func_start_address
- + (int32_t)size) & bitmask) > (pc & bitmask))
- {
- sframe_frame_row_entry_copy (frep, &cur_fre);
- return 0;
- }
+ sframe_frame_row_entry_copy (frep, &cur_fre);
+ return 0;
}
- else
- return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
+ fres += size;
}
return sframe_set_errno (&err, SFRAME_ERR_FDE_INVAL);
}