Hi all, Thanks for your reply.
We shouldn't even get to use the iterator if it's an unknown instruction. > The decoder should just advance dword by dword until it finds something > that > makes sense again. > Got it) So this is an expected behavior there: return iter_group_offset_bits(iter, iter->group_iter + 1) < > (gen_group_get_length(iter->group, iter->p) * 32); > when we convert a negative *int* to *uint* to return true to continue our loop. return iter_group_offset_bits(iter, iter->group_iter + 1) < > (*0xFFFFFFE0U*); > Do you think it is good idea to add comment or something like this into the "iter_more_groups" function: int *length* = gen_group_get_length(iter->group, iter->p); > return *length < 0 ||* > iter_group_offset_bits(iter, iter->group_iter + 1) < > (*length* * 32); > to show more explicitly here that we want to return true to continue our loop when the -1 is returned from the "gen_group_get_length" function because at the moment it is a bit implicit) Please let me know if I am incorrect. Regards, Andrii. On Tue, Aug 14, 2018 at 7:08 PM, Lionel Landwerlin < lionel.g.landwer...@intel.com> wrote: > On 14/08/18 16:16, Rafael Antognolli wrote: > >> On Tue, Aug 14, 2018 at 03:36:18PM +0100, Lionel Landwerlin wrote: >> >>> On 14/08/18 12:55, asimiklit.work wrote: >>> >>>> Hi Lionel, >>>> >>>>> Hi Andrii, >>>>> >>>>> Again sorry, I don't think this is the right fix. >>>>> I'm sending another patch to fix the parsing of >>>>> MI_BATCH_BUFFER_START which seems to be the actual issue. >>>>> >>>>> Thanks for working on this, >>>>> >>>> Thanks for your fast reply. >>>> I agree that it is not correct patch for this issue but anyway >>>> "iter_more_groups" function will still work incorrectly >>>> for unknown instructions when the "iter->group->variable" field is true. >>>> I guess that this case should be fixed. >>>> Please let me know if I am incorrect. >>>> >>> Hey Andrii, >>> >>> We shouldn't even get to use the iterator if it's an unknown instruction. >>> The decoder should just advance dword by dword until it finds something >>> that >>> makes sense again. >>> >>> If we run into that problem, I think we should fix the caller. >>> >> In that case, would an unreachable() or assert be a good thing to do? >> > > Yep, I guess assert in gen_field_iterator_init() should be a good thing. > > >> Regards, >>>> Andrii. >>>> >>>> On 2018-08-14 1:26 PM, Lionel Landwerlin wrote: >>>> >>>>> Hi Andrii, >>>>> >>>>> Again sorry, I don't think this is the right fix. >>>>> I'm sending another patch to fix the parsing of >>>>> MI_BATCH_BUFFER_START which seems to be the actual issue. >>>>> >>>>> Thanks for working on this, >>>>> >>>>> - >>>>> Lionel >>>>> >>>>> On 14/08/18 10:04, asimiklit.w...@gmail.com wrote: >>>>> >>>>>> From: Andrii Simiklit <asimiklit.w...@gmail.com> >>>>>> >>>>>> The "gen_group_get_length" function can return a negative value >>>>>> and it can lead to the out of bounds group_iter. >>>>>> >>>>>> v2: printing of "unknown command type" was added >>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107544 >>>>>> Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com> >>>>>> --- >>>>>> src/intel/common/gen_decoder.c | 13 +++++++++++-- >>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/src/intel/common/gen_decoder.c >>>>>> b/src/intel/common/gen_decoder.c >>>>>> index ec0a486..b36facf 100644 >>>>>> --- a/src/intel/common/gen_decoder.c >>>>>> +++ b/src/intel/common/gen_decoder.c >>>>>> @@ -770,6 +770,13 @@ gen_group_get_length(struct gen_group >>>>>> *group, const uint32_t *p) >>>>>> return -1; >>>>>> } >>>>>> } >>>>>> + 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; >>>>>> + } >>>>>> } >>>>>> return -1; >>>>>> @@ -803,8 +810,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; >>>>>> >>>>> >>>>> _______________________________________________ >>>> mesa-dev mailing list >>>> mesa-dev@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>>> >>> >>> >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
