Re: [PATCH i-g-t 02/10] lib/xe: Use stricter line-equality check when checking for workarounds

Gustavo Sousa <[email protected]> writes:

> Matt Roper <[email protected]> writes:
>
>> On Tue, Jul 21, 2026 at 03:59:55PM -0300, Gustavo Sousa wrote:
>>> Currently debugfs_file_has_wa() uses strstr() to check if a workaround
>>> name is present in the debugfs dump.  Using strstr() would match the
>>> workaround name anywhere in the dump buffer and with that we risk
>>> producing unexpected results if the checked workaround name happens to
>>> be a substring of another workaround present in the dump.
>>> 
>>> Fix that by making sure we match the workaround name with the full
>>> line from the dump.
>>
>> There are other parts of IGT that use regular expressions from either
>> POSIX (regcomp / regexec) or Glib (g_regex_new / g_regex_match).  Would
>> it be possible to use one of those here instead of opencoding a match
>> function?
>
> Yeah.  That could make this function more readable and easier to extend.
> I'll take a look at those options.

This new version of debugfs_file_has_wa() misses the fact that some
lines will contain multiple workarounds separated by comma.  So I
decided to implement a more robust version.  I have two versions, one
that is still not using regular expressions and another that uses POSIX
regcomp/regexec functions.  I have a slight preference for the former,
but if you prefer, we can go with the latter.  IMO, both versions are
more readable than the one currently proposed in this patch.

I would like your opinion before sending v2 of this series. :-)

Note: I also created a separate commit adding a simple test for this
function in lib/tests.

Version without regular expressions:

    | static bool is_eol(char c)
    | {
    |         return !c || c == '\n';
    | }
    | 
    | static bool is_wa_sep(char c)
    | {
    |         return isspace(c) || c == ',';
    | }
    | 
    | static bool debugfs_dump_has_wa(char *dump, const char *wa)
    | {
    |         size_t wa_len = strlen(wa);
    | 
    |         while (*dump) {
    |                 /*
    |                  * Each workaround name is indented by one tab
    |                  * character; unindented lines are used as "section
    |                  * names" identifying the type of workarounds that
    |                  * follow (e.g. "GT Workarounds", "Engine Workarounds"
    |                  * etc).
    |                  */
    |                 if (*dump != '\t')
    |                         goto next_line;
    | 
    |                 while (!is_eol(*dump)) {
    |                         char *other_wa;
    | 
    |                         while (!is_eol(*dump) && is_wa_sep(*dump))
    |                                 dump++;
    | 
    |                         other_wa = dump;
    | 
    |                         while (!is_eol(*dump) && !is_wa_sep(*dump))
    |                                 dump++;
    | 
    |                         if (wa_len == dump - other_wa &&
    |                             !strncmp(wa, other_wa, wa_len))
    |                                 return true;
    |                 }
    | 
    |         next_line:
    |                 while (!is_eol(*dump))
    |                         dump++;
    | 
    |                 if (*dump)
    |                         dump++;
    |         }
    | 
    |         return false;
    | }

Version with POSIX regcomp/regexec:

    | static int debugfs_dump_has_wa(char *dump, const char *wa)
    | {
    |         size_t wa_len;
    |         regex_t regex;
    |         regmatch_t match[2];
    |         int ret;
    | 
    |         if (regcomp(&regex, "[ \t,]*([^ \t,]+)", REG_EXTENDED | REG_NEWLINE)) {
    |                 igt_critical("Failed to compile regex, workaround %s will not be checked.\n",
    |                              wa);
    | 
    |                 return -1;
    |         }
    | 
    |         wa_len = strlen(wa);
    |         ret = 0;
    | 
    |         while (*dump) {
    |                 /*
    |                  * Each workaround name is indented by one tab
    |                  * character; unindented lines are used as "section
    |                  * names" identifying the type of workarounds that
    |                  * follow (e.g. "GT Workarounds", "Engine Workarounds"
    |                  * etc).
    |                  */
    |                 if (*dump != '\t')
    |                         goto next_line;
    | 
    |                 while (*dump && regexec(&regex, dump, 2, match, 0) == 0) {
    |                         char *other_wa = dump + match[1].rm_so;
    |                         size_t other_wa_len = match[1].rm_eo - match[1].rm_so;
    | 
    |                         dump += match[0].rm_eo;
    | 
    |                         if (wa_len == other_wa_len && !strncmp(wa, other_wa, wa_len)) {
    |                                 ret = 1;
    | 
    |                                 goto done;
    |                         }
    |                 }
    | 
    |         next_line:
    |                 while (*dump && *dump != '\n')
    |                         dump++;
    | 
    |                 if (*dump)
    |                         dump++;
    |         }
    | 
    | done:
    |         regfree(&regex);
    | 
    |         return ret;
    | }

--
Gustavo Sousa

>
> --
> Gustavo Sousa
>
>>
>>
>> Matt
>>
>>> 
>>> Signed-off-by: Gustavo Sousa <[email protected]>
>>> ---
>>>  lib/xe/xe_wa.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 38 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/lib/xe/xe_wa.c b/lib/xe/xe_wa.c
>>> index ff5daf529831..d44431e7e61f 100644
>>> --- a/lib/xe/xe_wa.c
>>> +++ b/lib/xe/xe_wa.c
>>> @@ -13,6 +13,43 @@
>>>  #include "xe/xe_wa.h"
>>>  #include "xe/xe_query.h"
>>>  
>>> +static bool debugfs_dump_has_wa(char *dump, const char *wa)
>>> +{
>>> +	char *a = dump;
>>> +
>>> +	while (*a) {
>>> +		const char *b = wa;
>>> +
>>> +		/*
>>> +		 * Each workaround name is indented by one tab
>>> +		 * character; unindented lines are used as "section
>>> +		 * names" identifying the type of workarounds that
>>> +		 * follow (e.g. "GT Workarounds", "Engine Workarounds"
>>> +		 * etc).
>>> +		 */
>>> +		if (*a++ != '\t')
>>> +			goto next_line;
>>> +
>>> +		while (*a == *b && !(*a == '\0' || *a == '\n' || *b == '\0')) {
>>> +			a++;
>>> +			b++;
>>> +		}
>>> +
>>> +		if ((*a == '\0' || *a == '\n') && *b == '\0')
>>> +			return true;
>>> +
>>> +	next_line:
>>> +		/* No match for this line, advance to the next one. */
>>> +		while (*a != '\n' && *a != '\0')
>>> +			a++;
>>> +
>>> +		if (*a == '\n')
>>> +			a++;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>>  static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
>>>  			       const char *debugfs_name, const char *wa)
>>>  {
>>> @@ -23,7 +60,7 @@ static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
>>>  
>>>  	debugfs_dump = igt_sysfs_get(debugfs_fd, debugfs_name);
>>>  	if (debugfs_dump) {
>>> -		char *has_wa = strstr(debugfs_dump, wa);
>>> +		bool has_wa = debugfs_dump_has_wa(debugfs_dump, wa);
>>>  
>>>  		free(debugfs_dump);
>>>  
>>> 
>>> -- 
>>> 2.55.0
>>> 
>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation