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

crossgen2: Add --imagebase option #65567

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

t-mustafin
Copy link
Contributor

--imagebase option set preferable ImageBase field to output PE-file

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 18, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-crossgen2-coreclr and removed community-contribution Indicates that the PR has been added by a community member labels Feb 18, 2022
@t-mustafin
Copy link
Contributor Author

cc @alpencolt @gbalykov

--imagebase option set preferable ImageBase field to output PE-file
@MichalStrehovsky
Copy link
Member

Looks reasonable to me but adding @dotnet/crossgen-contrib for actual signoff.

I do wonder whether the change you actually need is to just respect the ImageBase in the source executable (and set an ImageBase when compiling the source assembly). If I'm reading this right, this will set the same ImageBase for all components of a composite build and they'll be all guaranteed to conflict.

@t-mustafin
Copy link
Contributor Author

I do wonder whether the change you actually need is to just respect the ImageBase in the source executable (and set an ImageBase when compiling the source assembly). If I'm reading this right, this will set the same ImageBase for all components of a composite build and they'll be all guaranteed to conflict.

Actually we do not use composite mode, but use pipelene mode (--single-file-compilation --out-near-input). For pipeline mod I will prepare another patch.

@MichalStrehovsky
Copy link
Member

Actually we do not use composite mode, but use pipelene mode (--single-file-compilation --out-near-input). For pipeline mod I will prepare another patch.

I assume the command line interface will get ugly for the pipeline mode. Is there anything preventing you from setting the ImageBase on each of the inputs before running crossgen? The ImageBase of an assembly can be controlled at the csproj level of each assembly at the time of source build by using the <BaseAddress> property. If we made crossgen2 respect the BaseAddress of the input module instead of overwriting it with a new value, it would probably end up cleaner.

@t-mustafin
Copy link
Contributor Author

t-mustafin commented Mar 1, 2022

Is there anything preventing you from setting the ImageBase on each of the inputs before running crossgen? The ImageBase of an assembly can be controlled at the csproj level of each assembly at the time of source build by using the <BaseAddress> property.

I think here are three arguments to do not determine BaseAddress into .csproj file:

  1. BaseAddress is unique for each architecture, at least it differs for 32 and 64 bit modes.
  2. There are no information about ni sizes at time of writing .csproj file, which is used to determine BaseAddress value for next .csproj.
  3. Big enough changes of one source code could lead to rewriting of several .cproj files. c#->cil compiler will not warn about this need.

@jkotas
Copy link
Member

jkotas commented Mar 1, 2022

You can build a separate tool that goes over the universe of all assemblies and assigns them a base address based on some policy. crossgen can then pick the base address from each assembly in the pipeline mode, no command line option needed.

I understand that you are using fixed base addresses to save memory in Tizen specific builds via FEATURE_ENABLE_NO_ADDRESS_SPACE_RANDOMIZATION. Fixed base addresses suppress address space randomization that goes against our security roadmap, so it is not something we are likely to design a great experience for.

@trylek
Copy link
Member

trylek commented Apr 11, 2022

@t-mustafin - do you think that Jan's and Michal's suggestions are sufficient to address your needs or that there is still some value in making this change?

@gbalykov
Copy link
Member

You can build a separate tool that goes over the universe of all assemblies and assigns them a base address based on some policy

This approach leads to few problems which seem to be unresolvable for us.

  1. Independent teams that develop managed C# projects will have to use such a tool to sync used base addresses in csproj files, and this doesn't seem to be achievable.
  2. Update of base address in already existing dll on device might interfere with some existing dll signing policies.

Basically, what we would like to add is same as BaseAddress option of crossgen1 (dotnet/coreclr#25227).

For individual dll compilation this will be a base address to put in r2r image (just like in this PR and like it was for crossgen1):

./corerun ./crossgen2/crossgen2.dll -r:`pwd`/*.dll -o:$DLLPATH/$DLLNAME.ni.dll --imagebase $BASEADDR $DLLPATH/$DLLNAME.dll 

Resulting $DLLNAME.ni.dll will have $BASEADDR base address.

For the pipeline mode this will be an initial base address, on top of which base addresses for all compiled dlls will be calculated internally in crossgen2 (we'll add this changed later), i.e. also only one --imagebase option in command line:

./corerun ./crossgen2/crossgen2.dll -r:`pwd`/*.dll --out-near-input --single-file-compilation --imagebase $BASEADDR $DLLPATH1/$DLLNAME1.dll $DLLPATH2/$DLLNAME2.dll $DLLPATH3/$DLLNAME3.dll 

Resulting $DLLNAME1.ni.dll will have $BASEADDR base address, $DLLNAME2.ni.dll will have $BASEADDR + <smth> base address, $DLLNAME3.ni.dll will have $BASEADDR + <smth2> base address.

This way command line for both single-dll and pipeline modes will stay pretty simple.

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

As I believe that @gbalykov's explanation is reasonable and this change basically reintroduces functionality that was present in the legacy native Crossgen1, I believe it should be safe to take. I'll leave the PR open until tomorrow to see whether there's additional pushback from @jkotas or @MichalStrehovsky; in its absence I'll just merge it in tomorrow morning. Thanks!

@trylek trylek merged commit 6472398 into dotnet:main Apr 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2022
@t-mustafin t-mustafin deleted the cg2_imagebase_option_upsteam branch July 1, 2022 12:40
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