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.

[Marcus Shawcroft <marcus dot shawcroft at gmail dot com>]

Hi,

A few minor issues....

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