Re: [PATCH,v2] [AArch64] MTE corefile support

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-05-31 10:12 a.m., Luis Machado wrote:
>>> @@ -71,4 +72,18 @@ extern CORE_ADDR aarch64_mte_set_ltag (CORE_ADDR address, CORE_ADDR tag);
>>>      It is always possible to get the logical tag.  */
>>>   extern CORE_ADDR aarch64_mte_get_ltag (CORE_ADDR address);
>>>   +/* MTE-specific NT_MEMTAG header.  */
>>> +struct tag_dump_mte
>>> +{
>>> +  /* Size of the tag granule in bytes.  */
>>> +  uint16_t granule_byte_size;
>>> +  /* Size of the tag in bits.  */
>>> +  uint16_t tag_bit_size;
>>> +  /* Reserved field for the future.  */
>>> +  uint16_t __unused;
>>> +};
>>
>> This struct is unused.
>>
> 
> I'm inclined to leave that as documentation instead, so it is clear what the header sizes are referring to.

The thing is that somebody reading the code in the .c seeing the
arbitrary sizeofs  will note have a clue to go look at
arch/aarch64-mte-linux.h for explanations.  So another option would be
to write it as a comment near the code.  Something like:

  /* The header follows the following format:

     struct
     {
       /* Size of the tag granule in bytes.  */
       uint16_t granule_byte_size;
       /* Size of the tag in bits.  */
       uint16_t tag_bit_size;
       /* Reserved field for the future.  */
       uint16_t __unused;
     };
   */

Another option would be to have the structure and refer to its fields in
sizeof:

    store_unsigned_integer (buf, sizeof (tag_dump_mte::granule_byte_size),
			    byte_order, AARCH64_MTE_GRANULE_SIZE);

This has the advantage that the structure can still be present as documentation
in a central place, the reader of that code will naturally be pointed to
it if they need more information, and since the structure type is
actually used, it's fine to keep it there.

>> It's a little strange, NT_MEMTAG_HEADER_SIZE is defined at two places,
>> linux-tdep.h and here.  If this is really OS-independent, there should
>> probably be just one definition, in an OS-independent header file.  Not
>> sure where though...
>>
> 
> Right. I ran into this as well. I thought about this a bit, and I guess the simplest way is to have a new memtag.h header file and define it there.
> 
> It would be generic enough and other OS' would be able to include it as well. And it would work for Core files.

Sounds good.

>> The code of this function approximately up to here is really similar to
>> the code in corelow.c.  Is there a change there could be a single
>> implementation of it, that takes a callback?  Like
>>
>>    for_each_matching_memtag_note_section_matching_address
>>      (CORE_ADDR address, gdb::function_view<...> callback)
>>
>> except with a better name.
>>
> 
> I'm aware of that. I was hoping to leave this cleanup for a later stage.
> 
> A complicating factor here is that we need to traverse the NT_MEMTAG notes for two slightly different reasons.
> 
> The first is to decide if an address (no length specified) falls within one of the NT_MEMTAG entries, in which case we just return true/false;
> 
> The second is to actually fetch the tags the user has requested (via address + length). This may potentially span multiple NT_MEMTAG notes, so the callback may be invoked multiple times.
> 
> It should be possible to refactor the code to invoke callbacks, but we would need to have two callbacks (one for corelow and another for linux-tdep). The one for linux-tdep just needs to check if a note exists for a particular range, so it would be an useless callback in my opinion.
> 
> With that said, I reworked things and moved the common code to a new function. This new function goes through the notes and returns whenever we hit a note that contains the address we're looking for. Then we have a chance to do some processing, return or continue going through notes.
> 
> I think this simplifies things.

Ok, I'll see in the new version.

>> This struct is used once, in aarch64_linux_decode_memtag_note, but where
>> it's used it's not very useful, it could very well be replaced with 3
>> local variables, and then it could be removed.  I'm guessing that an
>> earlier version of this patch used this struct to read the data contents
>> directly from the buffer, but that didn't work with different byte
>> orders?
>>
> 
> Like the MTE NT_MEMTAG header, this is mostly for documentation purposes. We don't have a lot of use for the header structs right now, but may have more uses for them in the future. We may want to pass the header around and access it in a simpler way than just using local variables and skipping individual sizes.
> 
> This is because we're using a packed struct and we don't want to run into issues with alignments and padding inserted by the compilers, or having to handle endianness differences at the corelow.c level.

Yep.

> I'll do the same as I've done with the NT_MEMTAG_MTE_HEADER_SIZE case and will document this struct alongside the definition of NT_MEMTAG_HEADER_SIZE (in a new generic file, memtag.h).

As I suggested above, I think having the structure and referring to its
fields in sizeofs would be a nice solution.

Simon