x86: Initialize broadcast_op.bytes to 0

Message ID CAMe9rOpJ573hfc-3g1AqP5CQrayyR-993yhvqMqPDQbSALQLQQ@mail.gmail.com
State New
Headers show
Series
  • x86: Initialize broadcast_op.bytes to 0
Related show

Commit Message

H.J. Lu July 26, 2018, 3:51 p.m.
On Thu, Jul 26, 2018 at 8:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> Also, wouldn't you better clear ->bytes again in case the function later

>> returns an error, leading to the next template to be looked at?

>

> Good point.  I will take a look.

>


This is what I checked in.


-- 
H.J.

Comments

Jan Beulich July 26, 2018, 3:57 p.m. | #1
>>> On 26.07.18 at 17:51, <hjl.tools@gmail.com> wrote:

> On Thu, Jul 26, 2018 at 8:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>> Also, wouldn't you better clear ->bytes again in case the function later

>>> returns an error, leading to the next template to be looked at?

>>

>> Good point.  I will take a look.

>>

> 

> This is what I checked in.


Is that enough? This sets the value to zero once while parsing aiui,
but not between multiple attempts to match the parsing result
against templates.

Jan
H.J. Lu July 26, 2018, 4:01 p.m. | #2
On Thu, Jul 26, 2018 at 8:57 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.07.18 at 17:51, <hjl.tools@gmail.com> wrote:

>> On Thu, Jul 26, 2018 at 8:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>>> Also, wouldn't you better clear ->bytes again in case the function later

>>>> returns an error, leading to the next template to be looked at?

>>>

>>> Good point.  I will take a look.

>>>

>>

>> This is what I checked in.

>

> Is that enough? This sets the value to zero once while parsing aiui,

> but not between multiple attempts to match the parsing result

> against templates.

>


It should be sufficient. Do you have a testcase to show otherwise?
broadcast_op is referenced only by

              broadcast_op.type = bcst_type;
              broadcast_op.operand = this_operand;
              broadcast_op.bytes = 0;
              i.broadcast = &broadcast_op;

broadcast_op.bytes is initialized to 0 when broadcast_op.type
is set.


-- 
H.J.
Jan Beulich July 26, 2018, 4:12 p.m. | #3
>>> On 26.07.18 at 18:01, <hjl.tools@gmail.com> wrote:

> On Thu, Jul 26, 2018 at 8:57 AM, Jan Beulich <JBeulich@suse.com> wrote:

>>>>> On 26.07.18 at 17:51, <hjl.tools@gmail.com> wrote:

>>> On Thu, Jul 26, 2018 at 8:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>>>> Also, wouldn't you better clear ->bytes again in case the function later

>>>>> returns an error, leading to the next template to be looked at?

>>>>

>>>> Good point.  I will take a look.

>>>>

>>>

>>> This is what I checked in.

>>

>> Is that enough? This sets the value to zero once while parsing aiui,

>> but not between multiple attempts to match the parsing result

>> against templates.

>>

> 

> It should be sufficient. Do you have a testcase to show otherwise?


I don't have a testcase, I'm more afraid of this being a latent bug (i.e.
for someone to hit with future changes, just like I've recently had to
fix a long standing issue with found_reverse_match; patch yet to be
posted).

> broadcast_op is referenced only by

> 

>               broadcast_op.type = bcst_type;

>               broadcast_op.operand = this_operand;

>               broadcast_op.bytes = 0;

>               i.broadcast = &broadcast_op;

> 

> broadcast_op.bytes is initialized to 0 when broadcast_op.type

> is set.


That's all fine, but not to the point. If you fail matching against
a template after having set the field to non-zero already, the
matching attempt against the next template (if any) is going to
have this value already set, even if perhaps the template does
not allow for broadcast.

Jan
H.J. Lu July 26, 2018, 4:14 p.m. | #4
On Thu, Jul 26, 2018 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.07.18 at 18:01, <hjl.tools@gmail.com> wrote:

>> On Thu, Jul 26, 2018 at 8:57 AM, Jan Beulich <JBeulich@suse.com> wrote:

>>>>>> On 26.07.18 at 17:51, <hjl.tools@gmail.com> wrote:

>>>> On Thu, Jul 26, 2018 at 8:02 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>>>>> Also, wouldn't you better clear ->bytes again in case the function later

>>>>>> returns an error, leading to the next template to be looked at?

>>>>>

>>>>> Good point.  I will take a look.

>>>>>

>>>>

>>>> This is what I checked in.

>>>

>>> Is that enough? This sets the value to zero once while parsing aiui,

>>> but not between multiple attempts to match the parsing result

>>> against templates.

>>>

>>

>> It should be sufficient. Do you have a testcase to show otherwise?

>

> I don't have a testcase, I'm more afraid of this being a latent bug (i.e.

> for someone to hit with future changes, just like I've recently had to

> fix a long standing issue with found_reverse_match; patch yet to be

> posted).

>

>> broadcast_op is referenced only by

>>

>>               broadcast_op.type = bcst_type;

>>               broadcast_op.operand = this_operand;

>>               broadcast_op.bytes = 0;

>>               i.broadcast = &broadcast_op;

>>

>> broadcast_op.bytes is initialized to 0 when broadcast_op.type

>> is set.

>

> That's all fine, but not to the point. If you fail matching against

> a template after having set the field to non-zero already, the

> matching attempt against the next template (if any) is going to

> have this value already set, even if perhaps the template does

> not allow for broadcast.


I don't believe it will happen.  i.broadcast is initialized to zero
before parsing each instruction.  It is set to &broadcast_op only
when there is a broadcast operand.


-- 
H.J.

Patch

From 1f75763aa1bfe2f998f4347f0de436092a126980 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 26 Jul 2018 08:49:12 -0700
Subject: [PATCH] x86: Initialize broadcast_op.bytes to 0

	* config/tc-i386.c (check_VecOperations): Initialize
	broadcast_op.bytes to 0.
---
 gas/ChangeLog        | 5 +++++
 gas/config/tc-i386.c | 1 +
 2 files changed, 6 insertions(+)

diff --git a/gas/ChangeLog b/gas/ChangeLog
index 582a12d0ee..e8c500a2c0 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-07-26  H.J. Lu  <hongjiu.lu@intel.com>
+
+	* config/tc-i386.c (check_VecOperations): Initialize
+	broadcast_op.bytes to 0.
+
 2018-07-26  Alex Chadwick  <Alex.Chadwick@cl.cam.ac.uk>
 
 	* config/tc-ppc.c (md_show_usage): Add -mgekko and -mbroadway.
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 3f8bd93224..9e9c676580 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -8634,6 +8634,7 @@  check_VecOperations (char *op_string, char *op_end)
 
 	      broadcast_op.type = bcst_type;
 	      broadcast_op.operand = this_operand;
+	      broadcast_op.bytes = 0;
 	      i.broadcast = &broadcast_op;
 	    }
 	  /* Check masking operation.  */
-- 
2.17.1