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

Cache the path on Module for easier diagnostics #106103

Merged
merged 5 commits into from
Aug 9, 2024

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Aug 8, 2024

  • Make PEImage take a path in its constructor and make its member const
  • Store the path when initializing Module

I'm trying to avoid having to expose PEAssembly/PEImage in data descriptors to the cDAC just to get the module path.
Contributes to #99302

cc @lambdageek @davidwrighton

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@elinor-fung elinor-fung changed the title Cache the path on Module for eaiser diagnostics Cache the path on Module for easier diagnostics Aug 8, 2024
src/coreclr/vm/peimage.h Outdated Show resolved Hide resolved
src/coreclr/vm/ceeload.h Show resolved Hide resolved
src/coreclr/vm/ceeload.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/ceeload.cpp Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented Aug 8, 2024

Are we also trying to avoid exposing SString for the cdac?

SString works well for temporary values - method arguments, locals, etc. It works poorly as a field with longer lifetime that is potentially accessed by multiple threads (you have to be very careful how you use it to avoid subtle race conditions). I think it is a good idea to avoid exposing SString for the cdac

@AaronRobinsonMSFT
Copy link
Member

potentially accessed by multiple threads

@lambdageek You would think marking something const, as @elinor-fung did in this PR, would help mitigate some of those things. The issue is the SString type was written very badly and the designers applied const to methods and then when in the member function would cast away const. It is infuriating and means that applying const to an SString instance really doesn't mean much. For example, calling const UTF8 *GetUTF8() const; on an instance could mutate the instance and replace the UTF-16 encoding with a UTF-8 one while another thread is using it. I'm hoping I can get to it in .NET 10, but I've been saying that since .NET 8 :-(

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
@elinor-fung
Copy link
Member Author

Re: lifetime and const SString not really being const

  • Module ref counts PEAssembly which ref counts PEImage which has the path, so the underlying string should not go away for the lifetime of the Module
  • As @AaronRobinsonMSFT mentioned, making const actually mean const for SString is non-trivial. I just added an assert to Module::GetPath around the cached path matching the one in PEImage

@elinor-fung elinor-fung force-pushed the module-peimage-path branch from a65c3ce to b2062d4 Compare August 9, 2024 19:28
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Looks good to me. Long term, I'd like to see us stop using an SString as a field at all on PEImage, but we can do that some other time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants