Re: [PATCH] x86: reject architecture settings that are invalid to be set from the command line

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Jun 10, 2010 at 7:41 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>> On 10.06.10 at 16:32, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>> On Thu, Jun 10, 2010 at 7:25 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>>> On 10.06.10 at 16:19, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>> On Thu, Jun 10, 2010 at 12:05 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>>>>> On 09.06.10 at 18:02, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>>>> On Wed, Jun 9, 2010 at 8:36 AM, Jan Beulich <JBeulich@novell.com> wrote:
>>>>>>> So far, options like -march=i8086 were accepted despite the assembler
>>>>>>> subsequently choking on other consistency checks, leading to reasonably
>>>>>>> cryptic error messages. This patch makes it so that impossible
>>>>>>> architecure settings are neither accepted nor displayed (i.e. it is now
>>>>>>> made sure that those settings can only be used via directives).
>>>>>>>
>>>>>>> gas/
>>>>>>> 2010-06-09 ?Jan Beulich ?<jbeulich@novell.com>
>>>>>>>
>>>>>>> ? ? ? ?* config/tc-i386.c (md_parse_option): Ignore impossible processor
>>>>>>> ? ? ? ?types.
>>>>>>> ? ? ? ?(show_arch): New parameter 'check'.
>>>>>>> ? ? ? ?(md_show_usage): Adjust calls to show_arch().
>>>>>>>
>>>>>>> --- 2010-06-09/gas/config/tc-i386.c ? ? 2010-06-09 17:04:12.000000000 +0200
>>>>>>> +++ 2010-06-09/gas/config/tc-i386.c ? ? 2010-06-09 17:24:59.000000000 +0200
>>>>>>> @@ -8166,6 +8166,11 @@ md_parse_option (int c, char *arg)
>>>>>>> ? ? ? ? ? ? ?if (strcmp (arch, cpu_arch [j].name) == 0)
>>>>>>> ? ? ? ? ? ? ? ?{
>>>>>>> ? ? ? ? ? ? ? ? ?/* Processor. ?*/
>>>>>>> + ? ? ? ? ? ? ? ? if (! (strcmp (default_arch, "i386")
>>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ?? cpu_arch[j].flags.bitfield.cpulm
>>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ?: cpu_arch[j].flags.bitfield.cpui386))
>>>>>>> + ? ? ? ? ? ? ? ? ? continue;
>>>>>>> +
>>>>>>> ? ? ? ? ? ? ? ? ?cpu_arch_name = cpu_arch[j].name;
>>>>>>> ? ? ? ? ? ? ? ? ?cpu_sub_arch_name = NULL;
>>>>>>> ? ? ? ? ? ? ? ? ?cpu_arch_flags = cpu_arch[j].flags;
>>>>>>> @@ -8297,7 +8302,7 @@ md_parse_option (int c, char *arg)
>>>>>>> ?"
>>>>>> ? ? ?"
>>>>>>>
>>>>>>> ?static void
>>>>>>> -show_arch (FILE *stream, int ext)
>>>>>>> +show_arch (FILE *stream, int ext, int check)
>>>>>>> ?{
>>>>>>> ? static char message[] = MESSAGE_TEMPLATE;
>>>>>>> ? char *start = message + 27;
>>>>>>> @@ -8334,6 +8339,13 @@ show_arch (FILE *stream, int ext)
>>>>>>> ? ? ? ? ?/* It is an processor. ?Skip if we show only extension. ?*/
>>>>>>> ? ? ? ? ?continue;
>>>>>>> ? ? ? ?}
>>>>>>> + ? ? ?else if (check && ! (strcmp (default_arch, "i386")
>>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ?? cpu_arch[j].flags.bitfield.cpulm
>>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ?: cpu_arch[j].flags.bitfield.cpui386))
>>>>>>> + ? ? ? {
>>>>>>> + ? ? ? ? /* It is an impossible processor - skip. ?*/
>>>>>>> + ? ? ? ? continue;
>>>>>>> + ? ? ? }
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Do we need to check cpu_arch[j].flags.bitfield.cpulm? Can we
>>>>>> just check cpu_arch[j].flags.bitfield.cpui386 like
>>>>>>
>>>>>> if (check && !cpu_arch[j].flags.bitfield.cpui386)
>>>>>> ? continue?
>>>>>>
>>>>>
>>>>> I'm of the opinion that when the assembler is in 64-bit mode it
>>>>> should reject those architectures that aren't 64-bit capable,
>>>>> otherwise specifying e.g. -march=i386 has the same ugly effect
>>>>> as has passing -march=i8086 in 32-bit mode. And if we reject
>>>>> them, we should also not display them as available.
>>>>>
>>>>
>>>> On Linux/x86-64, your patch gave me
>>>>
>>>> ../as-new --help
>>>>
>>>> ? --32/--64 ? ? ? ? ? ? ? generate 32bit/64bit code
>>>> ? --divide ? ? ? ? ? ? ? ?ignored
>>>> ? -march=CPU[,+EXTENSION...]
>>>> ? ? ? ? ? ? ? ? ? ? ? ? ? generate code for CPU and EXTENSION, CPU is one
>>>> of:
>>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ?generic64, nocona, core2, corei7, l1om, opteron,
>>>> k8,
>>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ?amdfam10, bdver1
>>>>
>>>> I don't see how it can be correct since "--32 -march=i386" works fine.
>>>>
>>>
>>> Just try ../as-new --32 --help - it'll show the 32-bit possibilities
>>> as well.
>>
>> It doesn't look right to me.
>>
>
> I'm afraid I'm not following: Without --32, -march=i386 doesn't work.
> What's wrong then with not displaying the option in the first place
> without --32?

"--help" gives a list of available options. It doesn't mean all combinations
are valid.

> Or if you insist on your point, would you clarify how you think people
> can know that they can't pass -march=i386 without --32 on a 64-bit
> installation, when the help screen doesn't tell them so? If I can
> understand your thinking here, I could perhaps agree to not doing
> the check in the help function...

We already do:

[hjl@gnu-6 tmp]$ as -march=i386 -o x.o x.s
Assembler messages:
Error: 64bit mode not supported on this CPU.
x.s:8: Error: bad register name `%xmm0'
x.s:18: Error: bad register name `%xmm0'
x.s:28: Error: bad register name `%xmm0'
x.s:38: Error: bad register name `%xmm0'
x.s:48: Error: `xorpd' is not supported on `i386'
x.s:108: Error: bad register name `%xmm0'
x.s:110: Error: `addsd' is not supported on `i386'
x.s:164: Error: `cvtsi2sd' is not supported on `i386'
x.s:165: Error: `subsd' is not supported on `i386'
x.s:166: Error: `andpd' is not supported on `i386'
x.s:167: Error: `ucomisd' is not supported on `i386'
[hjl@gnu-6 tmp]$

You can turn

Error: 64bit mode not supported on this CPU.

into a fatal error and stop.


-- 
H.J.