Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UTC] Escape multiple {{ or }} in input for check lines. #71790

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Nov 9, 2023

SCEV expressions may contain multiple {{ or }} in the debug output, which needs escaping.

See llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll for a test that needs escaping.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 9, 2023

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-testing-tools

Author: Florian Hahn (fhahn)

Changes

SCEV expressions may contain multiple {{ or }} in the debug output, which needs escaping.

See llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll for a test that needs escaping.


Full diff: https://github.com/llvm/llvm-project/pull/71790.diff

1 Files Affected:

  • (modified) llvm/utils/UpdateTestChecks/common.py (+5)
diff --git a/llvm/utils/UpdateTestChecks/common.py b/llvm/utils/UpdateTestChecks/common.py
index 74044f925aadde4..e8d8d99257e81d0 100644
--- a/llvm/utils/UpdateTestChecks/common.py
+++ b/llvm/utils/UpdateTestChecks/common.py
@@ -1171,6 +1171,7 @@ def transform_line_vars(match):
         return match.group(1) + rv + match.group(match.lastindex)
 
     lines_with_def = []
+    multiple_braces_re = re.compile(r"(({{+)|(}}+))")
 
     for i, line in enumerate(lines):
         if not is_asm and not is_analyze:
@@ -1200,6 +1201,10 @@ def transform_line_vars(match):
                 (lines[i], changed) = nameless_value_regex.subn(
                     transform_line_vars, lines[i], count=1
                 )
+        if is_analyze:
+            # Escape multiple {{ or }} as {{}} denotes a FileCheck regex.
+            scrubbed_line = multiple_braces_re.sub(r"{{\\\1}}", lines[i])
+            lines[i] = scrubbed_line
     return lines
 
 

@nikic
Copy link
Contributor

nikic commented Nov 9, 2023

Can you please add a UTC test for this (or adjust an existing one to exercise this)?

fhahn added a commit that referenced this pull request Nov 9, 2023
SCEV expressions may contain multiple {{ or }} in the debug output,
which needs escaping.

See llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll
for a test that needs escaping.
@fhahn
Copy link
Contributor Author

fhahn commented Nov 9, 2023

Can you please add a UTC test for this (or adjust an existing one to exercise this)?

Thanks, added test in 129a91e.

@@ -81,7 +81,7 @@ define void @test_brace_escapes(ptr noundef %arr) {
; CHECK-NEXT: Grouped accesses:
; CHECK-NEXT: Group [[GRP4]]:
; CHECK-NEXT: (Low: {(64 + %arr),+,64}<%loop.1> High: {(8064 + %arr),+,64}<%loop.1>)
; CHECK-NEXT: Member: {{(64 + %arr),+,64}<%loop.1>,+,8}<%loop.2>
; CHECK-NEXT: Member: {{\{{}}(64 + %arr),+,64}<%loop.1>,+,8}<%loop.2>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct result? Shouldn't this be {{\{\{}}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for {{, but not for {{{ or more. I updated the code to use a replacement function that escapes each { in the sequence; not sure if there's some python regex magic that does that without a separate function

Copy link

github-actions bot commented Nov 9, 2023

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r 129a91ec01a975e73aaaa9b5010f4085f4ad3cb9..acbd908b61f81cd8bb180f363c167dedf7d0c38d llvm/utils/UpdateTestChecks/common.py
View the diff from darker here.
--- common.py	2023-11-09 17:05:52.000000 +0000
+++ common.py	2023-11-09 17:17:23.933791 +0000
@@ -1170,12 +1170,13 @@
         # including the commas and spaces.
         return match.group(1) + rv + match.group(match.lastindex)
 
     lines_with_def = []
     multiple_braces_re = re.compile(r"({{+)|(}}+)")
+
     def escape_braces(match_obj):
-        return '{{' + re.escape(match_obj.group(0)) + '}}'
+        return "{{" + re.escape(match_obj.group(0)) + "}}"
 
     for i, line in enumerate(lines):
         if not is_asm and not is_analyze:
             # An IR variable named '%.' matches the FileCheck regex string.
             line = line.replace("%.", "%dot")

Comment on lines 1176 to 1182
s = match_obj.group(0)
escaped = ''
if s[0] == '{':
escaped = s.replace('{', '\\{')
else:
escaped = s.replace('}', '\\}')
return ''.join(['{{', escaped, '}}'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
s = match_obj.group(0)
escaped = ''
if s[0] == '{':
escaped = s.replace('{', '\\{')
else:
escaped = s.replace('}', '\\}')
return ''.join(['{{', escaped, '}}'])
return re.escape(match_obj.group(0))

I think something like this would work? As we are actually escaping for use in a regex, the use of re.escape seems appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works well, we just need to manually add the wrapping {{ / }}

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

llvm/utils/UpdateTestChecks/common.py Outdated Show resolved Hide resolved
Co-authored-by: Nikita Popov <github@npopov.com>
@fhahn fhahn merged commit 24839c3 into llvm:main Nov 9, 2023
1 of 3 checks passed
@fhahn fhahn deleted the analyze-escape-braces branch November 9, 2023 17:18
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
SCEV expressions may contain multiple {{ or }} in the debug output,
which needs escaping.

See
llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll
for a test that needs escaping.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants