Skip to content

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
@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) <[email protected]>
@elinor-fung elinor-fung mentioned this pull request Aug 8, 2024
25 tasks
@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