Skip to content

Avoid a race condition in opt-viewer/optrecord #131214

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 2, 2025
Merged

Avoid a race condition in opt-viewer/optrecord #131214

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions llvm/tools/opt-viewer/optrecord.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,19 @@ class Remark(yaml.YAMLObject):

default_demangler = "c++filt -n"
demangler_proc = None
demangler_lock = Lock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I 100% agree with that.


@classmethod
def set_demangler(cls, demangler):
cls.demangler_proc = subprocess.Popen(
demangler.split(), stdin=subprocess.PIPE, stdout=subprocess.PIPE
)
cls.demangler_lock = Lock()

@classmethod
def demangle(cls, name):
with cls.demangler_lock:
if not cls.demangler_proc:
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this really need to be within the Lock?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the underlying problem is that before that patch, caller had to ensure set_demangler was called before calling demangle.
A neater way to enforce that would be to make demangler_proc lazily initialized, but the proposed approach is fine as demangler_proc is only referenced from demangle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it's outside of the lock, two workers could attempt to create the process.

  1. worker A sees demangler_proc is None
  2. worker B sees demangler_proc is None
  3. worked A assigns a new process to demangler_proc
  4. worker B assigns a new process to demangler_proc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was even getting stuff like:

  File "/builddir/build/BUILD/llvm-20.1.0-build/llvm-project-20.1.0.src/llvm/tools/opt-viewer/optrecord.py", line 160, in DemangledFunctionName
    return self.demangle(self.Function)
           ~~~~~~~~~~~~~^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/llvm-20.1.0-build/llvm-project-20.1.0.src/llvm/tools/opt-viewer/optrecord.py", line 85, in demangle
    cls.demangler_proc.stdin.write((name + "\n").encode("utf-8"))
    ^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'stdin'

cls.set_demangler(cls.default_demangler)
cls.demangler_proc.stdin.write((name + "\n").encode("utf-8"))
cls.demangler_proc.stdin.flush()
return cls.demangler_proc.stdout.readline().rstrip().decode("utf-8")
Expand Down Expand Up @@ -323,8 +325,6 @@ def get_remarks(input_file, filter_=None):
def gather_results(filenames, num_jobs, should_print_progress, filter_=None):
if should_print_progress:
print("Reading YAML files...")
if not Remark.demangler_proc:
Remark.set_demangler(Remark.default_demangler)
remarks = optpmap.pmap(
get_remarks, filenames, num_jobs, should_print_progress, filter_
)
Expand Down
Loading