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

On Wed, Aug 19, 2026 at 04:32:04PM -0300, Gustavo Sousa wrote:
> 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. :-)

It might be best to ping the IGT maintainers and see what they think.
Today IGT has a mix of open-coded string matching and two different
styles of regular expressions.  It seems like the kind of thing they may
want to set a standard direction for with new code.  I don't have much
of a personal opinion.


Matt

> 
> 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

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation