Re: [PATCH][ARM][1/2] Cleanup mixed use of "cpu_variant" and selected_cpu"

[Will Newton <will dot newton at linaro dot org>]

On 14 August 2014 10:03, Jiong Wang <jiong.wang@arm.com> wrote:
> currently, gas for arm32 have several variables to track cpu features:
>
> cpu_variant
> selected_cpu
> arm_arch_used
> thumb_arch_used
>
> and there are some wrong usage of cpu_variant and selected_cpu which
> actually affect
> binary correctness.
>
> I checked the log, there was cpu_variant only, but later a new variable
> "selected_cpu"
> was introduced for TAG_cpu attribute generation only, since then people
> start to
> use them in a mixed way.
>
> for selected_cpu, its value depends on arch/cpu options or directives, if
> none of
> them found, then its default value is zero.
>
> while for cpu_variant, its default value is a mask contains all features,
> and will be
> updated to the value of selected_cpu *if the value of selected_cpu is not
> zero*.
>
> so if no arch/cpu options/directives specified, then cpu_variant *accept*
> everything
> while *selected_cpu* reject everything.
>
> then if the code in parsing stage use cpu_variant, it will accept some
> features, but
> reject them in later fixup stage silently if this is not a error/warning
> report. this
> is a *logic error* affect correctness which should be fixed.
>
> for example, given the following .s
>
> .syntax unified
>
> .thumb
> .type entry, %function
> .global entry
> entry:
>   blx label           <=== A
>
> .type label, %function
> label:
>   bx  lr
>
> .arm
> .type label2, %function
> label2:
>   blx label3
>
> .type label3, %function
> label3:
>   bx  lr
>
> as -o 1.o 1.s
> objdump -d 1.o
>
> 00000000 <entry>:
>    0:   f7ff fffe       bl      0 <entry>
>
> at location A, a detected it's a thumb->thumb blx, then we convert it to bl,
> but the relocated
> destination point to itself which is wrong. this bug is just caused by mixed
> use of cpu_variant
> and selected_cpu.
>
> this patch and the following tries to clean up all the wrong usage of
> selected_cpu, to make
> feature check be consistent in the assembler.
>
> New variable usage
> =======================
>
>   * the first patch tries the following variable renaming:
>
>   s/cpu_variant/parse_arch/
>   s/selected_cpu_name/intended_arch_name/
>   s/selected_cpu/intended_arch/

Why are we replacing "cpu" with "arch"? Most of the manipulations of
these variables include with word "cpu" so it seems strange to name
them "arch".

>
>
>   * the second patch update intended_arch based on the info we got
>     during parsing, and thus naturally fix this bug.
>
>
>   "parse_arch" are to be used during gas parsing stage, for example those
>    do_XXX encoder in tc-arm.c.
>
>   "intended_arch" are to be used during all stage after parsing.
>
>   new variable calculation logic is:
>     * intended_arch default to be arm_arch_none which is empty.
>     * intended_arch initialized to the value of arch if any of
>       them is specified by cmdline or directive.
>
>     * if (inteneded_arch not empty) then
>         parse_arch = intended_arch
>       else
>         parse_arch = accept_all
>
>   the idea behind is if none arch info specified by the user, then
>   we should be permissive in parsing stage, while be strict in later
>   fixup stage. because accept unsupported instructions in parsing stage
>   will normally cause ill-instruction exception on silicon which is
>   obvious for user to fix, while wrong relocations may introduce silent
>   bugs which is hard to hunt.
>
>   any comments on this ?
>
>   thanks.
>
>   gas/
>     * config/tc-arm.c : Rename "cpu_variant" to "parse_arch",
>     "selected_cpu" to intended_arch", "selected_cpu_name" to
>     "intended_arch_name"



-- 
Will Newton
Toolchain Working Group, Linaro