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

[Jiong Wang <jiong dot wang at arm dot com>]

On 14/08/14 11:03, Will Newton wrote:
> 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".
will,

   thanks for your comment.

   it's OK to change "arch" back to "cpu".

   my intention is to find idea names that help people know immediately 
which one to use.

-- Jiong

> >    * 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"