Hi all,
Andrey also opened a bug about this issue :
https://bugs.freedesktop.org/show_bug.cgi?id=107544
It feels like it should be fixed on master though. get_length() shouldn't
return -1 for structs anymore.
We should probably return 1 at end of get_length() so that the decoder
prints out "unknown instruction".
That would help spot potential errors and updates needed to genxml.
1. I am not sure about 'return 1' at end of the 'get_length'
because it will be some kind of the lie for all callers of this function.
As far as i see this function should notify caller (using -1 in this case)
that the length can not be determinated due to some reason and
this gives the ability to the caller to decide what to do in this case.
The 'return 1' here will deprive us of this ability.
Please share any thoughts about it.
2. I also think that the "unknown command" message in the unknown command case
is very good idea.
I want to add the 'default' case to the switch at the end of 'get_length'
function:
default: {
fprintf(stderr, "Unknown command type %u in '%s::%s'\n",
type,
(group->parent && group->parent->name) ? group->parent->name :
"UNKNOWN",
group->name ? group->name : "UNKNOWN");
break;
}
Because in my case even if I return 1 at end of the 'get_length' function
I will not receive "unknown instruction" message
which you already added in 'gen_print_batch' function on line 808
because I came to this function another way.
What do you think about it?
I will send the updated patch as soon as we come to agreement.
Regards,
Andrii.
On 10.08.18 20:32, Rafael Antognolli wrote:
On Fri, Aug 10, 2018 at 05:37:12PM +0100, Lionel Landwerlin wrote:
Andrey also opened a bug about this issue :
https://bugs.freedesktop.org/show_bug.cgi?id=107544
It feels like it should be fixed on master though. get_length() shouldn't
return -1 for structs anymore.
We should probably return 1 at end of get_length() so that the decoder
prints out "unknown instruction".
That would help spot potential errors and updates needed to genxml.
Yeah, that makes sense. I saw the we were doing the check for length < 0
somewhere else so it seemed reasonable to check for that, considering we
can return -1, but I agree that printing "unknown instruction" would be
better.
-
Lionel
On 10/08/18 16:48, Rafael Antognolli wrote:
On Thu, Aug 09, 2018 at 03:00:30PM +0300, andrey simiklit wrote:
Hi,
Sorry I missed the main thought here.
The "gen_group_get_length" function returns int
but the "iter_group_offset_bits" function returns uint32_t
So uint32_t(int(-32)) = 0xFFFFFFE0U and it looks like unexpected behavior for
me:
iter_group_offset_bits(iter, iter->group_iter + 1) < 0xFFFFFFE0U;
That's fine, I think the original commit message is good enough to
understand this change. Feel free to add this extra bit too if you want,
but I don't think it's needed.
Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>
Regards,
Andrii.
On Thu, Aug 9, 2018 at 2:35 PM, Andrii Simiklit <asimiklit.w...@gmail.com>
wrote:
The "gen_group_get_length" function can return a negative value
and it can lead to the out of bounds group_iter.
Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com>
---
src/intel/common/gen_decoder.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder
.c
index ec0a486..f09bd87 100644
--- a/src/intel/common/gen_decoder.c
+++ b/src/intel/common/gen_decoder.c
@@ -803,8 +803,10 @@ static bool
iter_more_groups(const struct gen_field_iterator *iter)
{
if (iter->group->variable) {
- return iter_group_offset_bits(iter, iter->group_iter + 1) <
- (gen_group_get_length(iter->group, iter->p) * 32);
+ const int length = gen_group_get_length(iter->group, iter->p);
+ return length > 0 &&
+ iter_group_offset_bits(iter, iter->group_iter + 1) <
+ (length * 32);
} else {
return (iter->group_iter + 1) < iter->group->group_count ||
iter->group->next != NULL;
--
2.7.4
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
