This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


>>> 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.

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?
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).

Jan



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]