[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:mattst88 at gmail.com] 
Sent: Monday, November 13, 2017 9:02 PM
To: Rogovin, Kevin <kevin.rogovin at intel.com>
Cc: mesa-dev at 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.rogovin at intel.com> wrote:
> From: Kevin Rogovin <kevin.rogovin at 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.rogovin at 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?