Re: [PATCH 3/6] x86: improve operand reversal

["Jan Beulich" <JBeulich at suse dot com>]

>>> On 06.08.18 at 14:54, <hjl.tools@gmail.com> wrote:
> On Sun, Aug 5, 2018 at 11:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 03.08.18 at 18:14, <hjl.tools@gmail.com> wrote:
>>> On Fri, Aug 3, 2018 at 9:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 03.08.18 at 17:56, <hjl.tools@gmail.com> wrote:
>>>>> On Fri, Aug 3, 2018 at 8:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 03.08.18 at 17:30, <hjl.tools@gmail.com> wrote:
>>>>>>> On Fri, Aug 3, 2018 at 12:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>> On 02.08.18 at 18:43, <hjl.tools@gmail.com> wrote:
>>>>>>>>> Please don't make any changes to the deprecated ".s".
>>>>>>>>
>>>>>>>> Excuse me, but how many more times should I state that I don't
>>>>>>>> make any changes to its behavior? I solely make {store} no longer
>>>>>>>> match .s in behavior. In fact I've specifically undone the change
>>>>>>>> to .s, in anticipation of your objection to any adjustment to it.
>>>>>>>
>>>>>>> Why do you change testcases of the .s suffix?
>>>>>>
>>>>>> s/change/add to/
>>>>>>
>>>>>> Deprecated or no, I think the .s suffix should still work, including
>>>>>> not causing assembly to fail when used. Try assembling the
>>>>>> additions without the source adjustments, and I think you'll find
>>>>>> some will fail. To me such adjustments are not "changes to its
>>>>>
>>>>> Which one?
>>>>
>>>> The additions to *opts.s. For example, this
>>>>
>>>>         mov 0x12345678, %eax
>>>>         mov.s 0x12345678, %eax
>>>>         mov %eax, 0x12345678
>>>>         mov.s %eax, 0x12345678
>>>>         mov 0x123456789abcdef0, %eax
>>>>         mov.s 0x123456789abcdef0, %eax
>>>>         mov %eax, 0x123456789abcdef0
>>>>         mov.s %eax, 0x123456789abcdef0
>>>>         movabs 0x123456789abcdef0, %eax
>>>>         movabs.s 0x123456789abcdef0, %eax
>>>>         movabs %eax, 0x123456789abcdef0
>>>>         movabs.s %eax, 0x123456789abcdef0
>>>>         mov %eax, (%rdi)
>>>>         mov.s %eax, (%rdi)
>>>>         mov (%rdi), %eax
>>>>         mov.s (%rdi), %eax
>>>>         mov %cr0, %rax
>>>>         mov.s %cr0, %rax
>>>>         mov %rax, %cr7
>>>>         mov.s %rax, %cr7
>>>>         mov %dr0, %rax
>>>>         mov.s %dr0, %rax
>>>>         mov %rax, %dr7
>>>>         mov.s %rax, %dr7
>>>>
>>>> doesn't assemble with 2.31.1 (several "unsupported instruction"
>>>> and one "operand size mismatch").
>>>
>>> I saw
>>>
>>> s.s:4: Error: unsupported instruction `mov'
>>> s.s:6: Error: unsupported instruction `mov'
>>> s.s:10: Error: operand size mismatch for `movabs'
>>> s.s:14: Error: unsupported instruction `mov'
>>> s.s:18: Error: unsupported instruction `mov'
>>> s.s:22: Error: unsupported instruction `mov'
>>>
>>> There is no need to fix these since the ".s" suffix has been deprecated.
>>
>> "There is no need" can mean several things:
>> 1) You want me to deliberately remove the code correction (which
>> may result in overall less readable / maintainable code).
>> 2) You want me to simply not add tests for the corrected behavior
>> (which would be contrary to your general position that ideally any
>> code change would be accompanied by a test).
>> 3) I can leave the patch the way it is, as it also doesn't mean the
>> behavior must not be fixed.
>> And perhaps more.
> 
> I don't want to make any changes to assembler, whose sole purposes
> are to change/improve the behavior of the ".s"  suffix.
> 
>> Based on what I've written before, I'm opposed to 1, I could live
>> with 2, but I'd much prefer 3.
>>
>> A further note on deprecation of these suffixes: By looking at just
>> the source code, how did you expect me to know they're deprecated?
> 
> Can you recommend a good way to indicate that?

A code comment in the section responsible for the parsing of these
suffixes?

>> There was no comment whatsoever added to that effect back when
>> you've made that change, neither to the .c file nor to the respective
>> test cases (which, if you suggest to go with 2, should perhaps be
>> removed altogether).
> 
> I have removed the ".s" suffix from the assembler manual, but kept
> the testcases to avoid breaking existing codes.  But we shouldn't
> make further changes to improve it.

But you realize that the fixing of .s is more a byproduct of fixing
{load} and {store}? That's why I keep saying that _not_ fixing
.s at the same time would likely result in worse (and hence harder
to maintain) code. And that's also why I'm considering option 2
above tolerable, albeit not ideal.

Jan