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

[BOLT] Use restored names in funcs-{,file-}no-regex #127738

Conversation

aaupov
Copy link
Contributor

@aaupov aaupov commented Feb 19, 2025

Since function names given via funcs-no-regex are supposed to be verbatim
and don't contain BOLT-added suffixes, the check should use "restored"
BinaryFunction names stripped of suffixes.

Test Plan: added funcs-no-regex.s

Created using spr 1.3.4
@llvmbot
Copy link
Member

llvmbot commented Feb 19, 2025

@llvm/pr-subscribers-bolt

Author: Amir Ayupov (aaupov)

Changes

Since function names given via funcs-no-regex are supposed to be verbatim
and don't contain BOLT-added suffixes, the check should use "restored"
BinaryFunction names stripped of suffixes.

Test Plan: added funcs-no-regex.s


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

2 Files Affected:

  • (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+5-5)
  • (added) bolt/test/X86/funcs-no-regex.s (+16)
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 4329235d47049..13e7e52dc5691 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -2976,8 +2976,7 @@ void RewriteInstance::selectFunctionsToProcess() {
   populateFunctionNames(opts::FunctionNamesFileNR, opts::ForceFunctionNamesNR);
 
   // Make a set of functions to process to speed up lookups.
-  std::unordered_set<std::string> ForceFunctionsNR(
-      opts::ForceFunctionNamesNR.begin(), opts::ForceFunctionNamesNR.end());
+  StringSet<> ForceFunctionsNR(opts::ForceFunctionNamesNR);
 
   if ((!opts::ForceFunctionNames.empty() ||
        !opts::ForceFunctionNamesNR.empty()) &&
@@ -3051,9 +3050,10 @@ void RewriteInstance::selectFunctionsToProcess() {
           return true;
 
       // Non-regex check (-funcs-no-regex and -funcs-file-no-regex).
-      for (const StringRef Name : Function.getNames())
-        if (ForceFunctionsNR.count(Name.str()))
-          return true;
+      if (Function.forEachName([&](StringRef Name) {
+            return ForceFunctionsNR.contains(NameResolver::restore(Name));
+          }))
+        return true;
 
       return false;
     }
diff --git a/bolt/test/X86/funcs-no-regex.s b/bolt/test/X86/funcs-no-regex.s
new file mode 100644
index 0000000000000..a2cf8c1e91ce7
--- /dev/null
+++ b/bolt/test/X86/funcs-no-regex.s
@@ -0,0 +1,16 @@
+## Checks handling of function names passed via -funcs-no-regex
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
+# RUN: ld.lld %t.o -o %t.exe -nostdlib
+# RUN: llvm-bolt %t.exe -o %t.out -funcs-no-regex=func -print-cfg | FileCheck %s
+# CHECK: Binary Function "func/1"
+
+.globl _start
+.type _start, @function
+_start:
+  ret
+  .size _start, .-_start
+
+.type func, @function
+func:
+  ud2

@maksfb
Copy link
Contributor

maksfb commented Feb 19, 2025

Since function names given via funcs-no-regex are supposed to be verbatim
and don't contain BOLT-added suffixes, the check should use "restored"
BinaryFunction names stripped of suffixes.

How are you going to achieve fine-grained control over local function selection?


# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
# RUN: ld.lld %t.o -o %t.exe -nostdlib
# RUN: llvm-bolt %t.exe -o %t.out -funcs-no-regex=func -print-cfg | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use double dash for verbose options.

@aaupov
Copy link
Contributor Author

aaupov commented Feb 19, 2025

Since function names given via funcs-no-regex are supposed to be verbatim
and don't contain BOLT-added suffixes, the check should use "restored"
BinaryFunction names stripped of suffixes.

How are you going to achieve fine-grained control over local function selection?

Good point. Looks like it's better to fix bughunter script instead to achieve this, and emphasize that we expect suffixed names in all *funcs* options

@aaupov aaupov closed this Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants