Page MenuHomePhabricator

Florian (Florian)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 12 2020, 7:58 AM (66 w, 1 d)

Recent Activity

Jul 1 2020

Florian added a comment to D82342: Preserve GlobalsAA analysis result in LowerConstantIntrinsics .

@Florian since we've given it a couple days, would it be ok to make the commit now?

Jul 1 2020, 1:32 PM · Restricted Project

Apr 8 2020

Florian updated the diff for D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..

I integrated your feedback and put a separate callback for just the clean up. To not do any further refactoring I placed it in OnFinished where also other resources of ThreadState are freed.
The cleanup accesses non of the previously freed resources and does not free any resources needed in the destructor called after it or the stat aggregation, which are the only functions accessing ThreadState afterwards.

Apr 8 2020, 9:12 AM · Restricted Project, Restricted Project

Apr 3 2020

Florian updated the diff for D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..

Making the ThreadFinish call into platform specific code caused all kinds of further issues.
Now the ThreadState itself owns and manages the ThreadSignalContext, as it is required on all supported platforms anyways.

Apr 3 2020, 7:29 AM · Restricted Project, Restricted Project

Mar 26 2020

Florian added a comment to D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..

It appears this broke the LLDB bot: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/13127/testReport/junit/

Can you please take a look?

Florian, please take a look. There are some crashes on darwin.

Mar 26 2020, 11:22 AM · Restricted Project, Restricted Project

Mar 25 2020

Florian added a comment to D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..

Lastly, if I plan to make changes, is it ok to introduce features like virtual classes and smart pointers (on non hot paths) or are there restrictions on these in the codebase?

If it results in a real improvement and does not degrade performance of fast paths.
Fast paths were crafted very carefully to avoid any single excessive instruction. A virtual call on most fast paths is unacceptable.
Also it needs to make things simpler rather than harder. For example, reference counting may make things more obscure and non-transparent, lead to leaks and ordering bugs. Though, of course, absence of refcounting may lead to all the same problems :)
So I guess it will mostly depend on case-by-case basis. For example, we use scoped locks extensively and that's very handy and I guess prevented lots of bugs.

Speaking of unnecessary complexity and refactoring.

The current design of ThreadRegistry always confused me. Reading this change, it makes it very hard to follow logic.

We call ThreadFinish, which calls thread_registry->FinishThread which is in different directory, which always unconditionally calls virtual ThreadContext::OnFinished which is in a different part of tsan_rtl_thread.cc. I always need to open 5 files and do constant switching just to recover this part each time.

We need to remove OnFinished entirely and just do whatever we do there after the thread_registry->FinishThread call right in ThreadFinish.

Mar 25 2020, 5:54 AM · Restricted Project, Restricted Project
Florian added a comment to D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..

Would it be a worthwhile effort for me to pull responsibilities of logical and 'real' threads apart?

Yes. The Processor entity was introduced rather late in runtime evolution and only few most critical things were moved to the Processor from ThreadState. Perhaps more things can be moved.
Note tsan is also used for Go where Processor maps to a "logical processor" (a unit of GOMAXPROCS) and ThreadState maps to a goroutine. That was the main motivation for the split.

Mar 25 2020, 5:22 AM · Restricted Project, Restricted Project

Mar 24 2020

Florian added a comment to D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..

Hello Dmitry,

Mar 24 2020, 4:48 AM · Restricted Project, Restricted Project
Florian updated the diff for D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..
  • The clean-up callback for the platform code is moved to a different part of the thread destruction sequence.
  • Convert the test case to C instead of C++
Mar 24 2020, 4:16 AM · Restricted Project, Restricted Project

Mar 12 2020

Florian created D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers..
Mar 12 2020, 9:13 AM · Restricted Project, Restricted Project