This is the mail archive of the
mailing list for the binutils project.
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.
- From: Marcus Shawcroft <marcus dot shawcroft at gmail dot com>
- To: Philipp Tomsich <philipp dot tomsich at theobroma-systems dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>, Andrew Pinski <pinskia at gmail dot com>
- Date: Tue, 7 Jan 2014 12:49:43 +0000
- Subject: 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.
- Authentication-results: sourceware.org; auth=none
- References: <CA+=Sn1nxGT+x9JgJo=B6iOBKcOz0xWiF5nf2t=6vgVz+eMSpDQ at mail dot gmail dot com> <1388666881-20727-1-git-send-email-philipp dot tomsich at theobroma-systems dot com>
A few minor issues....
On 2 January 2014 12:48, Philipp Tomsich
> + 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.
> -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)
> - 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);
> - 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.