Re: [PATCH/AArch64 2/4] Remove the artificial limit on code alignment through the use of the fixed part of a fragment for output generation only, which required MAX_MEM_FOR_RS_ALIGN_CODE to be large enough to hold the maximum pad.

[Andrew Pinski <pinskia at gmail dot com>]

On Tue, Jan 7, 2014 at 4:49 AM, Marcus Shawcroft
<marcus.shawcroft@gmail.com> wrote:
> Hi,
>
> A few minor issues....

Any news on this patch since it never went in?  Could you also remove
md_do_align too?

Right now even a simple:
start64:
 nop
 .align 6
 done:
  nop

is broken.

Thanks,
Andrew Pinski


>
> On 2 January 2014 12:48, Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>
>> +   Here we fill the frag with the appropriate info for padding the output
>> +   stream. The resulting frag will consist of a fixed (fr_fix) and of a
>
> GNU style places two spaces after a period.  Same comment applies
> through the patch.
>
>> +   repeating (fr_var) part.
>> +
>> +   The fixed content is always emitted before the repeating content and
>> +   these two parts are used as follows in constructing the output:
>> +   - the fixed part will be used to align to a valid instruction word
>> +     boundary, in case that we start at a misaligned address; as no
>> +     executable instruction can live at the misaligned location, we
>> +     simply fill with zeros;
>> +   - the variable part will be used to cover the remaining padding and
>> +     we fill using the AArch64 NOP instruction.
>> +
>> +   Note that the size of a RS_ALIGN_CODE fragment is always 7 to provide
>> +   enough storage space for up to 3 bytes for padding the back to a valid
>> +   instruction alignment and exactly 4 bytes to store the NOP pattern.
>> + */
>
> Remove the trailing white space throughout the patch please.
>
>>
>>  void
>> -aarch64_handle_align (fragS * fragP)
>> +aarch64_handle_align (fragS * frag)
>
> Please don't mix rename /  reformat with functional changes, it makes
> it harder to review the patch.  In this case fragP -> frag is an
> unrelated change, either split the patch into two one with a rename
> the other with the functional change, or drop the rename completely.
>
>>  {
>>    /* NOP = d503201f */
>>    /* AArch64 instructions are always little-endian.  */
>
> Double space after period.
>
>> @@ -5765,69 +5783,32 @@ aarch64_handle_align (fragS * fragP)
>>
>>    int bytes, fix, noop_size;
>>    char *p;
>> -  const char *noop;
>>
>> -  if (fragP->fr_type != rs_align_code)
>> +  if (frag->fr_type != rs_align_code)
>>      return;
>>
>> -  bytes = fragP->fr_next->fr_address - fragP->fr_address - fragP->fr_fix;
>> -  p = fragP->fr_literal + fragP->fr_fix;
>> -  fix = 0;
>> -
>> -  if (bytes > MAX_MEM_FOR_RS_ALIGN_CODE)
>> -    bytes &= MAX_MEM_FOR_RS_ALIGN_CODE;
>> +  bytes = frag->fr_next->fr_address - frag->fr_address - frag->fr_fix;
>> +  p = frag->fr_literal + frag->fr_fix;
>>
>>  #ifdef OBJ_ELF
>> -  gas_assert (fragP->tc_frag_data.recorded);
>> +  gas_assert (frag->tc_frag_data.recorded);
>>  #endif
>>
>> -  noop = aarch64_noop;
>> -  noop_size = sizeof (aarch64_noop);
>> -  fragP->fr_var = noop_size;
>> +  noop_size = sizeof(aarch64_noop);
>
> Inappropriate removal of the white space before the opening parenthesis.
>
> Thanks
> /Marcus