Re: A lean way for getting the size of the instruction at a given address

[Luis Machado <luis.machado@linaro.org>]

Zied,

On 4/5/21 7:12 PM, Zied Guermazi wrote:
> Hi Luis,
> 
> yes, it guess it was intended for processing disassemble command. Itwas 
> not intended to be used in performance critical use cases. Once it was 
> removed, the next bottle neck is the printf in 
> get_all_disassembler_options ( a string was used as a mean for passing 
> options). it consumes 20% of the time.
> 
> Shall we put the changes needed to increase the performance in the "etm 
> for branch tracing" patch set, or in a dedicated one (performance 
> improvement one). please advicse

This would be best as a separate patch. It will be easier to review that 
way.

You may need to submit the change to both gdb/binutils lists, if the 
patch touches both projects.

> 
> /Zied
> 
> On 06.04.21 00:04, Luis Machado wrote:
>> Hi Zied,
>>
>> On 4/5/21 6:47 PM, Zied Guermazi wrote:
>>> hi Luis,
>>>
>>> thanks for your support. To experiment the impact of removing the 
>>> printing of the instruction on the overall performance, I commented 
>>> out setting and using the print function pointer in print_insn 
>>> (bfd_vma pc, struct disassemble_info *info, bfd_boolean little) in 
>>> opcodes/arm-dis.c, and the result was very interesting: The time 
>>> needed to process the traces dropped down from 12 minutes to 34 
>>> seconds for 64 MB of traces.
>>
>> That is quite a bottleneck! I think this code path isn't exercised often.
>>
>>>
>>> now that we have a proof that the bottleneck was printing, we can 
>>> think about a way to provide a clean implementation.
>>
>> I agree. A faster implementation of this particular function would be 
>> nice to have. It may even improve some other code paths that use this 
>> information.
>>
>>>
>>> Kind Regards
>>>
>>> Zied Guermazi
>>>
>>>
>>> On 05.04.21 18:40, Luis Machado wrote:
>>>> On 4/5/21 1:17 PM, Zied Guermazi wrote:
>>>>> hi Luis
>>>>>
>>>>> A new member function in "class gdb_disassembler" to calculate the 
>>>>> instruction length only will be a good solution. In fact a big 
>>>>> overhead is added by the printing of instruction disassembly, which 
>>>>> is not needed at all. On aarch64, the decoder is optimized to issue 
>>>>> many instruction in one trace element, and here calculating the 
>>>>> size consumes more than 80% of the time. On arm, the decoder issues 
>>>>> one instruction after another and here getting the size consumes 
>>>>> 50% of the time. Considering the amount of traces this can sum up 
>>>>> to a dozen of minutes in some cases (64MB of traces)
>>>>
>>>> Indeed, that doesn't sound good.
>>>>
>>>>>
>>>>> Calculating the instruction size per se, on arm is a "rapid" 
>>>>> operation and consists of checking few bits in the opcode. So the 
>>>>> time can be drastically decreased by having a function to calculate 
>>>>> the size only.
>>>>>
>>>>>
>>>>> gdb_print_insn can be then changed as following (pseudo code):
>>>>>
>>>>> int
>>>>> gdb_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
>>>>>          struct ui_file *stream, int *branch_delay_insns)
>>>>> {
>>>>>
>>>>>    gdb_disassembler di (gdbarch, stream);
>>>>>
>>>>>    if ( di.get_insn_size != 0)
>>>>>
>>>>>     return di.get_insn_size(memaddr);
>>>>>
>>>>>    else
>>>>>
>>>>>     return di.print_insn (memaddr, branch_delay_insns);
>>>>> }
>>>>>
>>>>> Is there a function in aarch64-tdep or arm-tdep doing job of 
>>>>> disassembly ( the lower layer handling the opcode)? are we relaying 
>>>>> on the bfd library for it? can someone give me a hint of where to 
>>>>> find those functions?
>>>>
>>>> The gdbarch hooks in arm-tdep.c (gdb_print_insn_arm) and 
>>>> aarch64-tdep.c (aarch64_gdb_print_insn) are more like helper 
>>>> functions and do some initial setup, but the code to disassemble 
>>>> lies in opcodes/arm-dis.c (print_insn) and opcodes/aarch64-dis.c 
>>>> (print_insn_aarch64).
>>>>
>>>> If you go with the route of changing "class gdb_disassembler", then 
>>>> you'll probably need to touch binutils/opcodes.
>>>>
>>>> If you decide to have a gdbarch hook (in arm-tdep/aarch64-tdep), 
>>>> then you only need to change GDB.
>>>>>
>>>>>
>>>>> Kind Regards
>>>>>
>>>>> Zied Guermazi
>>>>>
>>>>>
>>>>> On 05.04.21 15:01, Luis Machado wrote:
>>>>>> Hi Zied,
>>>>>>
>>>>>> On 4/4/21 4:59 AM, Zied Guermazi wrote:
>>>>>>> hi
>>>>>>>
>>>>>>> I need to get the size of the instruction at a given address. I 
>>>>>>> am currently using gdb_insn_length (struct gdbarch *gdbarch, 
>>>>>>> CORE_ADDR addr) which calls gdb_print_insn (struct gdbarch 
>>>>>>> *gdbarch, CORE_ADDR memaddr, struct ui_file *stream, int 
>>>>>>> *branch_delay_insns). and this is consuming a huge time, 
>>>>>>> considering that this is used in branch tracing and this gets 
>>>>>>> repeated up to few millions times.
>>>>>>>
>>>>>>>
>>>>>>> Is there a lean way for getting the size of the instruction at a 
>>>>>>> given address, I am using it for aarch64 and arm targets.
>>>>>>
>>>>>> At the moment I don't think there is an optimal solution for this. 
>>>>>> The instruction length is calculated as part of the disassemble 
>>>>>> process, and is tied to the function that prints instructions.
>>>>>>
>>>>>> One way to speed things up is to have a new member function in 
>>>>>> "class gdb_disassembler" to calculate the instruction length only.
>>>>>>
>>>>>> Another way is to have a new gdbarch hook that calculates the size 
>>>>>> of an instruction based on the current PC, mapping symbols etc.
>>>>>>
>>>>>>>
>>>>>>> Kind Regards
>>>>>>>
>>>>>>> Zied Guermazi
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>