Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument on annotating error

[Rogovin, Kevin]

Thankyou! very much for the patch to the command line disassembler.

 -Kevin

-Original Message-
From: Matt Turner [mailto:matts...@gmail.com] 
Sent: Friday, November 17, 2017 6:52 AM
To: Rogovin, Kevin <kevin.rogo...@intel.com>
Cc: mesa-dev@lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument 
on annotating error

On Wed, Nov 15, 2017 at 12:13 AM, Rogovin, Kevin <kevin.rogo...@intel.com> 
wrote:
> I have just seen that I have had an epic brain lapse on this.
>
> The code is pretty clear, the correct value of count should be ann_count - i. 
> This is because:
>   a. The value of ann_count is the value of the array size BEFORE the insert; 
> this is clear from the code within the if (offset + ..) where it increments 
> ann_count.
>  b. Since ann_count is the size before the insert, it should move the content 
> in the open range [i, ann_count) to [i + 1, ann_count + 1); thus the number 
> of elements is given by ann_count - i.
>
> Changing the count to ann_count - i does the right thing, and also makes it 
> quite clear that Matt is correct on patch 11: that it was just papering over 
> a bug from using the wrong count value.

I think I used the wrong data structure :)

I reproduced the problem you were having and then got tired of thinking about 
how to memmove elements. I rewrote the annotation code yesterday to use a 
linked list, which is much more natural. I just sent the patches. Attached is a 
patch you can squash into your series in order to make it work on top of my 
series.

> However, once this is done, the build does assert() on some shaders that I 
> have; this is because it fails to understand some registers.

I think that's a result of the disassembler not knowing how to disassemble 
sends/sendsc. Both Toni and Neil have written patches for that. I'll see if I 
can rebase them.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument on annotating error

[Matt Turner]

On Wed, Nov 15, 2017 at 12:13 AM, Rogovin, Kevin
 wrote:
> I have just seen that I have had an epic brain lapse on this.
>
> The code is pretty clear, the correct value of count should be ann_count - i. 
> This is because:
>   a. The value of ann_count is the value of the array size BEFORE the insert; 
> this is clear from the code within the if (offset + ..) where it increments 
> ann_count.
>  b. Since ann_count is the size before the insert, it should move the content 
> in the open range [i, ann_count) to [i + 1, ann_count + 1); thus the number 
> of elements is given by ann_count - i.
>
> Changing the count to ann_count - i does the right thing, and also makes it 
> quite clear that Matt is correct on patch 11: that it was just papering over 
> a bug from using the wrong count value.

I think I used the wrong data structure :)

I reproduced the problem you were having and then got tired of
thinking about how to memmove elements. I rewrote the annotation code
yesterday to use a linked list, which is much more natural. I just
sent the patches. Attached is a patch you can squash into your series
in order to make it work on top of my series.

> However, once this is done, the build does assert() on some shaders that I 
> have; this is because it fails to understand some registers.

I think that's a result of the disassembler not knowing how to
disassemble sends/sendsc. Both Toni and Neil have written patches for
that. I'll see if I can rebase them.


p
Description: Binary data
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument on annotating error

[Rogovin, Kevin]

I have just seen that I have had an epic brain lapse on this.

The code is pretty clear, the correct value of count should be ann_count - i. 
This is because:
  a. The value of ann_count is the value of the array size BEFORE the insert; 
this is clear from the code within the if (offset + ..) where it increments 
ann_count.
 b. Since ann_count is the size before the insert, it should move the content 
in the open range [i, ann_count) to [i + 1, ann_count + 1); thus the number of 
elements is given by ann_count - i.

Changing the count to ann_count - i does the right thing, and also makes it 
quite clear that Matt is correct on patch 11: that it was just papering over a 
bug from using the wrong count value.

However, once this is done, the build does assert() on some shaders that I 
have; this is because it fails to understand some registers.

-Kevin

-Original Message-
From: Matt Turner [mailto:matts...@gmail.com] 
Sent: Monday, November 13, 2017 11:21 PM
To: Rogovin, Kevin <kevin.rogo...@intel.com>
Cc: mesa-dev@lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument 
on annotating error

On Mon, Nov 13, 2017 at 1:12 PM, Rogovin, Kevin <kevin.rogo...@intel.com> wrote:
> Hi,
>
>
>  I confess I am not 100% on this code and I did educated guessing what it is  
> trying to do; I figured it was trying to insert contents at the current index 
> i; and that ann_count is the size -after- the insert. thus I figured the 
> memmove is to move the half open interval [i, ann_count-1) to the half open 
> interval [i + 1, ann_count). The number of elements in the half open range 
> [i, ann_count - 1) is given by ann_count - i - 1.
>
> I tried changing the count from ann_count - i - 1 to ann_count - i + 1 and 
> then the disassembler crashed in annotation on the same shaders that I have 
> had it crash on before.

Interesting. Could you send me the shader binary? I'll take a look.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument on annotating error

[Matt Turner]

On Mon, Nov 13, 2017 at 1:12 PM, Rogovin, Kevin  wrote:
> Hi,
>
>
>  I confess I am not 100% on this code and I did educated guessing what it is  
> trying to do; I figured it was trying to insert contents at the current index 
> i; and that ann_count is the size -after- the insert. thus I figured the 
> memmove is to move the half open interval [i, ann_count-1) to the half open 
> interval [i + 1, ann_count). The number of elements in the half open range 
> [i, ann_count - 1) is given by ann_count - i - 1.
>
> I tried changing the count from ann_count - i - 1 to ann_count - i + 1 and 
> then the disassembler crashed in annotation on the same shaders that I have 
> had it crash on before.

Interesting. Could you send me the shader binary? I'll take a look.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument on annotating error

[Rogovin, Kevin]

Hi,


 I confess I am not 100% on this code and I did educated guessing what it is  
trying to do; I figured it was trying to insert contents at the current index 
i; and that ann_count is the size -after- the insert. thus I figured the 
memmove is to move the half open interval [i, ann_count-1) to the half open 
interval [i + 1, ann_count). The number of elements in the half open range [i, 
ann_count - 1) is given by ann_count - i - 1.

I tried changing the count from ann_count - i - 1 to ann_count - i + 1 and then 
the disassembler crashed in annotation on the same shaders that I have had it 
crash on before.

 -Kevin

-Original Message-
From: Matt Turner [mailto:matts...@gmail.com] 
Sent: Monday, November 13, 2017 9:02 PM
To: Rogovin, Kevin <kevin.rogo...@intel.com>
Cc: mesa-dev@lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument 
on annotating error

On Mon, Nov 13, 2017 at 5:17 AM,  <kevin.rogo...@intel.com> wrote:
> From: Kevin Rogovin <kevin.rogo...@intel.com>
>
> Without this fix, disassembling of GEN shaders with GPU commands that 
> the disassembler does not know would result in errors being added to 
> the annotator which would crash when more than one error was added.
>
> Signed-off-by: Kevin Rogovin <kevin.rogo...@intel.com>
> ---
>  src/intel/compiler/intel_asm_annotation.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/intel_asm_annotation.c 
> b/src/intel/compiler/intel_asm_annotation.c
> index b07a545a12..7aa222f04e 100644
> --- a/src/intel/compiler/intel_asm_annotation.c
> +++ b/src/intel/compiler/intel_asm_annotation.c
> @@ -181,8 +181,9 @@ annotation_insert_error(struct annotation_info 
> *annotation, unsigned offset,
>   continue;
>
>if (offset + sizeof(brw_inst) != next->offset) {
> - memmove(next, cur,
> - (annotation->ann_count - i + 2) * sizeof(struct 
> annotation));
> + int count;
> + count = annotation->ann_count - i - 1;
> + memmove(next, cur, count * sizeof(struct annotation));

I don't see how this can be right.

Take for example a case where we have ann_count == 4. We have
annotation->ann[0..4] where ann[4] is the end marker... a little
surprising. We want to insert an error annotation on an instruction in ann[2] 
but not at the end, so we need to split ann[2].

cur = 2, next = 3. We need to memmove elements 2, 3, 4 one spot later, and then 
update ann[2] and ann[3] (which after the memmove is a copy of ann[2]).

Count should be ann_count (4) - i (2) + 1 -> 3. The code currently says +2 
instead of +1 and that seems like a bug.

What do you think?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument on annotating error

[Matt Turner]

On Mon, Nov 13, 2017 at 5:17 AM,   wrote:
> From: Kevin Rogovin 
>
> Without this fix, disassembling of GEN shaders with GPU commands
> that the disassembler does not know would result in errors being
> added to the annotator which would crash when more than one error
> was added.
>
> Signed-off-by: Kevin Rogovin 
> ---
>  src/intel/compiler/intel_asm_annotation.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/intel_asm_annotation.c 
> b/src/intel/compiler/intel_asm_annotation.c
> index b07a545a12..7aa222f04e 100644
> --- a/src/intel/compiler/intel_asm_annotation.c
> +++ b/src/intel/compiler/intel_asm_annotation.c
> @@ -181,8 +181,9 @@ annotation_insert_error(struct annotation_info 
> *annotation, unsigned offset,
>   continue;
>
>if (offset + sizeof(brw_inst) != next->offset) {
> - memmove(next, cur,
> - (annotation->ann_count - i + 2) * sizeof(struct 
> annotation));
> + int count;
> + count = annotation->ann_count - i - 1;
> + memmove(next, cur, count * sizeof(struct annotation));

I don't see how this can be right.

Take for example a case where we have ann_count == 4. We have
annotation->ann[0..4] where ann[4] is the end marker... a little
surprising. We want to insert an error annotation on an instruction in
ann[2] but not at the end, so we need to split ann[2].

cur = 2, next = 3. We need to memmove elements 2, 3, 4 one spot later,
and then update ann[2] and ann[3] (which after the memmove is a copy
of ann[2]).

Count should be ann_count (4) - i (2) + 1 -> 3. The code currently
says +2 instead of +1 and that seems like a bug.

What do you think?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 07/18] intel/compiler: fix for memmove argument on annotating error

[kevin . rogovin]

From: Kevin Rogovin 

Without this fix, disassembling of GEN shaders with GPU commands
that the disassembler does not know would result in errors being
added to the annotator which would crash when more than one error
was added.

Signed-off-by: Kevin Rogovin 
---
 src/intel/compiler/intel_asm_annotation.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/intel/compiler/intel_asm_annotation.c 
b/src/intel/compiler/intel_asm_annotation.c
index b07a545a12..7aa222f04e 100644
--- a/src/intel/compiler/intel_asm_annotation.c
+++ b/src/intel/compiler/intel_asm_annotation.c
@@ -181,8 +181,9 @@ annotation_insert_error(struct annotation_info *annotation, 
unsigned offset,
  continue;
 
   if (offset + sizeof(brw_inst) != next->offset) {
- memmove(next, cur,
- (annotation->ann_count - i + 2) * sizeof(struct annotation));
+ int count;
+ count = annotation->ann_count - i - 1;
+ memmove(next, cur, count * sizeof(struct annotation));
  cur->error = NULL;
  cur->error_length = 0;
  cur->block_end = NULL;
-- 
2.14.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev