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

Add compatibility attribute when linker is used as analyzer tool #1269

Closed
marek-safar opened this issue Jun 12, 2020 · 22 comments
Closed

Add compatibility attribute when linker is used as analyzer tool #1269

marek-safar opened this issue Jun 12, 2020 · 22 comments
Labels
Milestone

Comments

@marek-safar
Copy link
Contributor

This is a proposal to add global assembly attribute for assemblies which linker processes as an analyzer. The attribute could be something like ILLinkCompatibility ("{mode}", generated: true) which would indicate that during the assembly processing illinker didn't find any problems and are safe to be linked. The rules what it means exactly would need to be defined but it should include things like no warnings from flow-analysis, no usage of linker incompatible APIs, etc. The linker would not add this attribute if added manually by the user.

The mode value would reflect the highest linker compatibility level and would make to linker actions, specifically

  • link
  • copy
  • copyused

The usage scenario would then be for any NuGet packages to be automatically linker enabled without doing any manual work on packages or references level.

@eerhardt
Copy link
Member

I considered opening a new issue for the write-up below, but I felt this proposal is close enough to this discussion, that I decided to add it to this issue:

Problem

With Blazor WASM in .NET 5 we shipped an untrimmed System.IO.Pipelines.dll assembly in the default Blazor app. See dotnet/aspnetcore#27782. In 3.2, Blazor had the rule any assembly whose name starts with System. will get trimmed. Moving to the official .NET SDK in 5.0 changed this logic. By default, the .NET SDK will only trim assemblies that are shipped in the Microsoft.NETCore.App "runtimepack" (or think "Shared Framework"). There are a number of System. assemblies that aren't packaged into the Microsoft.NETCore.App "runtimepack", but instead dotnet/runtime only ships them as a NuGet packages. For example:

  • System.CodeDom (in WindowsDesktop shared fx)
  • System.Composition.*
  • System.Configuration.ConfigurationManager (in WindowsDesktop shared fx)
  • System.Diagnostics.EventLog (in ASP.NET shared fx)
  • System.Diagnostics.PerformanceCounter
  • System.DirectoryServices (in WindowsDesktop shared fx)
  • System.Drawing.Common (in WindowsDesktop shared fx)
  • System.Formats.Cbor
  • System.IO.Packaging (in WindowsDesktop shared fx)
  • System.IO.Pipelines (in ASP.NET shared fx)
  • System.IO.Ports
  • System.Management
  • System.Net.Connections
  • System.Net.Http.WinHttpHandler
  • System.Net.WebSockets.WebSocketProtocol
  • System.Reflection.MetadataLoadContext
  • System.Resources.Extensions (in WindowsDesktop shared fx)
  • System.Runtime.Caching
  • System.Security.Cryptography.Pkcs (in ASP.NET & WindowsDesktop shared fx)
  • System.Security.Cryptography.Xml (in ASP.NET & WindowsDesktop shared fx)
  • System.Security.Permissions (in ASP.NET & WindowsDesktop shared fx)
  • System.Threading.AccessControl (in WindowsDesktop shared fx)
  • System.Windows.Extensions (in ASP.NET & WindowsDesktop shared fx)

If a trimmed app references any of the above assemblies that come from dotnet/runtime, they won't be trimmed by default.

Solution alternatives

We should consider a model where a NuGet package can some how opt into being trimmed by default. That way a user of a package doesn't have to "guess" if an assembly is safe for trimming, and having to mark the assembly as "trimmable" themselves.

The above is one proposal for how to do this. However, with the complete system we have today, we will have some problems with the assembly level attribute. The reason being that the .NET SDK marks every assembly that isn't marked as "trimmable" as -p copy on the command line. It wouldn't be a great design for the assembly attribute to override a command line arg. So we would either need to teach the SDK targets a different command line to invoke the ILLinker, or we would need to teach the SDK targets to crack open and inspect each assembly for the attribute.

Another potential issue with the top post's proposal is generating an assembly level attribute dynamically needs to be done in such a way that it works with "deterministic" builds. Since we don't have the ILLinker used as an analyzer tool yet, I'm not sure how this would work - if it would be a pre or post compile step. Just something to ensure we handle correctly.

Another alternative is to use the MSBuild extensibility we added in 5.0: dotnet/sdk#12035. In this solution, each NuGet package would need to ship a .targets file that hooks into the PrepareForILLink target, and marks themselves as themselves as TrimMode=link. While this can work, it feels unnecessarily complex and too much of a pain to ask NuGet library authors to ship a pre-canned .targets file.

Another alternative would be to add a new flag to the NuGet nuspec file that says "all the managed libraries in this package should be opted into trimming by default". We have similar metadata in NuGet packages - for example the serviceable attribute. However, it might not be great to add an ILLinker specific attribute directly into a nuspec file. We could also consider having a "sentinel" file in the nuget package that the SDK sees, and uses as "this assembly is trimmable".

Thoughts

I think the assembly level attribute approach has the best possibility of working. However, we will need to figure out how to make it work correctly with the SDK targets and command line options. This may drastically change the way we invoke the ILLinker today.

We can take baby-steps to get there:

  1. Define the new assembly level attribute in dotnet/runtime
  2. Modify the mono/linker to respect the new attribute in such a way that the SDK targets can invoke the ILLinker and attributed assemblies get trimmed by default.
  3. Modify the dotnet/sdk to pass the newly formulated command line args to the ILLinker.

Then eventually, when we have the ILLinker running as an analyzer, we can figure out how to dynamically generate this attribute, if necessary.

I think we need to figure out a path forward here in .NET 6. There are more libraries that need to be trimmed by default than what is in the Microsoft.NETCore.App runtimepack today.

cc @vitek-karas @sbomer @MichalStrehovsky

@marek-safar marek-safar added this to the .NET 6.0 milestone Nov 13, 2020
@MichalStrehovsky
Copy link
Member

Do we expect that going forward we'll keep the behavior that framework assemblies get trimmed by default, and everything else gets rooted?

Trimmability is not a property of an assembly - attribution that declares "I don't dynamically use any of my own code" is not very useful - there might still be code in another assembly that dynamically uses code in this assembly.

We applied such rule to framework assemblies because maybe it's a bit less likely that someone will be reflecting on the framework, but it's just a heuristic with no guarantees of actually helping. There are assemblies within framework where this is more likely to work, but also assemblies where it's less likely (e.g. assemblies that have classes that might be considered "data that is likely to be reflection-serialized" vs assemblies that have mostly code in them and don't represent data/state).

For .NET 6 I would hope we'll look more into directions where linker can prove correctness, as opposed to investing in heuristics that may result in broken apps.

@marek-safar
Copy link
Contributor Author

The reason being that the .NET SDK marks every assembly that isn't marked as "trimmable" as -p copy on the command line. It wouldn't be a great design for the assembly attribute to override a command line arg.

I agree this setup is not ideal but we can certainly improve it. My suggestion would be to not treat the shared framework assemblies differently to any other library. If we standardized to assembly-level attribute we can add it to all libraries including shared framework assemblies. Then we don't pass any -p option to linker and leave the default behaviour to the attribute presence. The assembly link action would still be overrideable with existing -p if the user wants.

There is already attribute for that called LinkerSafeAttribute which we'll have to migrate to .NET6 for compatibility. I'm not saying we have to use this one but it'd make the migration for mobile easier.

Do we expect that going forward we'll keep the behavior that framework assemblies get trimmed by default, and everything else gets rooted?

No, we should make it possible to trim any library and in .NET6 we want to trim a couple of SDKs fully as well.

For .NET 6 I would hope we'll look more into directions where linker can prove correctness, as opposed to investing in heuristics that may result in broken apps.

I don't think it would be a good experience to run analyzer before linking all the times on libraries which are "static" for the end developer. We should run the verification at the end of the library build, that means once. If there are no issues detected it could be automatically marked as fully trimmable. Presumably, this could be done during build time via one analyzer but this probably needs more thoughts and it should not block us to allows this to be achieved manually.

@vitek-karas
Copy link
Member

To me it seems we're mixing two concepts here:

  • Asking SDK/linker to fully trim a subset of assemblies because we've found that this is likely to work and helps with size. The discussion is how to define the set (hardcoded in SDK, some external annotation, or internal (assembly attribute) annotation). Note that this is in all cases heuristic - just like @MichalStrehovsky points out above. This has basically nothing to do with correctness.
  • Somehow annotate assembly that it doesn't produce warnings on its own - thus potentially saving work for the linker. If this would work properly it's a purely perf optimization. Again no impact on correctness really. We would obviously need numbers for this, but the analysis cost on its own is probably small - linker still has to do most of its work in constant propagation and marking on all assemblies, regardless of such annotation.

Blazor and Xamarin both want to use the ability to define a set of assemblies to trim - as a heuristic to improve application sizes without breaking them too much.

We were hoping that in 6 we would be able to invest into framework and some nugets to actually fix the correctness issue - which is not addressed by any of the above. There will be some work, but probably not enough to get us to a place where we can have true correctness for some end to end scenario (like Blazor or Xamarin). As such it's likely we will need to keep the heuristics around even for 6.

Based on the description above - I would stay away from names like "linker safe", "trim compatible" or anything similar for the proposed attribute. The attribute would be a technical way how to define the set of assemblies to apply aggressive trimming to in our heuristic scenarios. Nothing more, or less. I can see a world where we add it to even an assembly which still produces some linker warnings on its own, although it would be nice to not do that.

Let me reiterate: adding such attribute to an assembly doesn't make it safe to trim - the question if something is safe to trim or not can only be answered globally, not on per-assembly level.

So maybe a name like DefaultTrimAction("link") or similar.

@vitek-karas
Copy link
Member

As for what the SDK should do - I think the current behavior is weird around assemblies like System.IO.Pipelines - at least in the Blazor case. It passes in -a System.IO.Pipelines which will root everything in that assembly AND annotate it with "copy" action. It then passes -p copy System.IO.Pipelines which will give it the "copy" action again.

The really "weird" part comes for the rest of the assemblies. The default action is set to "link" and then it singles out every assembly it doesn't know about (anything but frameworks). That is a really weird approach given that "link" is the most aggressive action. I would do it the other way round - default to the "safest" action, so probably "copy" and then single out framework assemblies with -p link System.Xml.dll.

Once we have the attribute discussed here, we could omit the -p and rely on the assembly attributes instead. The priority should definitely be: -p > attribute > -c/-u

If I understand how actions work in the linker, the -a should only be used on the application assembly itself. Since it's meant to mark "entry points".

@marek-safar
Copy link
Contributor Author

So maybe a name like DefaultTrimAction("link") or similar.

I like the name or we could go with DefaultTrimMode to match msbuild property.

@sbomer
Copy link
Member

sbomer commented Nov 13, 2020

I see the action as a decision made about how to link a particular assembly at link time - not a property of the assembly. After all, we want different actions for the wasm runtime than for the desktop runtime. As such I find it a bit strange to bake it in as an attribute.

I would do it the other way round - default to the "safest" action, so probably "copy" and then single out framework assemblies with -p link System.Xml.dll
Once we have the attribute discussed here, we could omit the -p and rely on the assembly attributes instead.

The issue with this is that the global TrimMode which can be set in the project file needs to affect the default for trimmed assemblies - it doesn't make sense for it to be a default for non-trimmed assemblies. If we do this with -p, it would override the action in the assembly attributes.

Putting the action in an assembly attribute would also require us to crack open every assembly just in case its action is copy, which goes against the direction we are heading with lazy loading. I would suggest not supporting copy in the assembly attribute.

-a should only be used on the application assembly itself. Since it's meant to mark "entry points".

We pass -a for non-trimmed assemblies to ensure that their dependencies are also kept.

@sbomer
Copy link
Member

sbomer commented Nov 16, 2020

Putting the action in an assembly attribute would also require us to crack open every assembly just in case its action is copy

I realized it's worse than this - if we want to keep assemblies that haven't opted into trimming via an attribute, we will need to scan them all - regardless of how we pass options on the command-line.

Considering some scenarios where SDK options interact with the attributes:

  • Developer sets <TrimMode>link</TrimMode> to run the linker in a more aggressive mode for trimmed assemblies

    To support this, there needs to be an attribute opt-in that respects the default TrimMode (-c/-u). Something like [DefaultTrimMode("default")] with a better name. :)

  • Developer sets <IsTrimmable>true</IsTrimmable> metadata on an assembly to opt it into trimming.

    Shouldn't change the action if the assembly specifies [DefaultTrimMode("<action>")].
    Should use TrimMode if the assembly doesn't specify [DefaultTrimMode("<action>")].
    We would need a new command-line option for this. Maybe -p default <assembly>.

  • Developer sets <TrimMode>link</TrimMode> metadata on an assembly to opt it into trimming.

    Passes -p link <assembly>. Overrides any action in the attribute.

  • Assemblies without an attribute or IsTrimmable/TrimMode metadata are expected to get rooted and copied

    Instead of pre-scanning for attributes in the SDK and passing -p copy <assembly> and -a <assembly> for everything without an attribute, the linker should know about these defaults.

I'd like to keep this as simple as possible. I think that in the most common scenarios, a developer (framework author or external) simply wants to enable trimming of their assembly or of an assembly dependency of their app. Why don't we just support an unparameterized attribute like [IsTrimmable] (name TBD) that doesn't specify the action? If later we find a need to specify the action we can always extend it to take a string.

The semantics would be that explicit actions (-p) have the highest priority, and otherwise [IsTrimmable] assemblies get the TrimMode behavior. <IsTrimmable> metadata can continue passing -p actions (we don't need -p default).

Either way we still need the ability to root and keep everything that isn't opted into trimming.

@marek-safar
Copy link
Contributor Author

marek-safar commented Nov 18, 2020

To summarize your proposed logic the linker will be altered to follow these rules

  1. if Assembly is listed with -p option on the command line
    -> is always taken for the assembly
  2. then if assembly includes global IsTrimmable attribute
    -> assembly action is link
  3. then if the global default action is set for the illink via command line (we merge -c and -u options into single new option)
    -> assembly uses the new global option value as the action
  4. then fallback to a hardcoded value
    -> we set copy to the assembly action

Does it sound right?

@sbomer
Copy link
Member

sbomer commented Nov 18, 2020

I would clarify a couple points. I think it's the same as what you are saying, but I want to be sure:

  1. Assembly includes global IsTrimmable attribute
    -> assembly action is the default action if given on the command line, otherwise link
  2. Global default action is set for the illink via command line (we merge -c and -u options into single new option)
    -> assembly uses the new global option value as the action if it has IsTrimmable, otherwise it getscopy behavior

@vitek-karas
Copy link
Member

Sorry to start a bit high-level, but I think it will make sense that way: Goals:

  • SDKs should be able to choose which assemblies are trimmed aggressively by default and which are not trimmed aggressively
    • For example Blazor would choose to aggressively trim all framework assemblies, but none of the app's assemblies
  • SDKs should be able to choose what "aggressive trimming" means by default
    • For example Blazor would choose "link" as the aggressive trimming, but netcoreapp console app (currently) chooses "copyused"
  • Libraries should be able to choose if they opt-in to "aggressive trimming" - probably via attribute (requiring build integration for all NuGets which want to do this is not ideal)
  • Developer should be able to hand pick additional assemblies to opt-in to "aggressive trimming" for a given app
  • Developer should be able to override this and opt-in all assemblies to "aggressive trimming" if they choose to - ideally without listing all assemblies in the app (the current story around this is too complex to do)
  • Developer should be able to override what "aggressive trimming" means for the app - both more aggressive or less aggressive than the SDK default
  • Supporting complete mix mode, where some assemblies are "copy", some are "copyused" and some are "link" is probably not necessary - and we don't need first class support for them. It should be possible to do this on the linker command line though.

So to this end (didn't think it through all the details yet, but in general):

  • Attribute which opts an assembly into aggressive trimming - I don't like IsTrimmable (all assemblies are potentially trimmable), but SDK calls this IsTrimmable currently.
  • The existing IsTrimmable metadata in SDK on assemblies - which opts the assemblies in to aggressive trimming. I would also prefer to have a separate item list with simple names of assemblies. It's much easier to set that in the app.csproj, the current approach requires custom target and some magic.
  • TrimMode defines what aggressive trimming means - this basically maps to current behavior well.
  • Potentially add something like TrimAllAssemblies which would automatically opt all assemblies into aggressive trimming

It's very similar to how it works today, the main difference is the attribute which acts basically identically to the SDK's IsTrimmable metadata. In terms of priorities:

  • If the assembly has IsTrimmable=true in SDK, it will be aggressively trimmed
  • If the assembly has IsTrimmable=false in SDK, it will NOT be aggressively trimmed
  • If IsTrimmable metadata is not specified, and the assembly has IsTrimmable attribute, it will be aggressively trimmed
  • If neither is specified it will not be trimmed

We should agree on the behavior first, and then we can design how this maps to linker command line.

We can basically start with the lists defined in SDKs the way it is now, an migrate over to having the attributes in the assemblies and SDKs not specifying the list as we go.

Long term - SDKs will in time ideally migrate to TrimAllAssemblies=true and TrimMode=link as the defaults, leaving the IsTrimmable metadata as a way for the developer to explicitly opt-out.

@sbomer
Copy link
Member

sbomer commented Nov 20, 2020

I think that's generally a good summary of the behavior we want. This is pretty much the key point:

the main difference is the attribute which acts basically identically to the SDK's IsTrimmable metadata

Note on terminology - the way I think about it, IsTrimmable opts into trimming, whether that's aggressive (TrimMode link) or not (TrimMode copyused). I'd replace "aggressive trimming" in your description with "trimming".

It sounds like there are two additional suggestions:

  • Allow opting into trimming via an ItemGroup, not just via IsTrimmable metadata
  • A property that simplifies opting everything into trimming: TrimAllAssemblies

Both sound like good usability improvements.

@marek-safar
Copy link
Contributor Author

In terms of priorities:

If the assembly has IsTrimmable=true in SDK, it will be aggressively trimmed
If the assembly has IsTrimmable=false in SDK, it will NOT be aggressively trimmed

I'm not sure this is the right order. I think the assembly attribute should win over SDK so a new assembly version does not require SDK update if there is a problem with the assembly.

I'm also a bit concern about the expansion of the settings. From what I gathered there will be following settings which can alter assembly trimming

  • IsTrimmableAttribute (true/false/not-present)
  • IsTrimmable msbuild metadata (true/false/not-present)
  • TrimMode msbuild property (link/copy/copyused)
  • AssemblyTrimMode msbuild property (link/copy/copyused)
  • TrimAllAssemblies msbuild property (link/copy/copyused)

Do we need all these to alter the trimming?

@sbomer
Copy link
Member

sbomer commented Nov 20, 2020

I opened https://github.com/dotnet/sdk/issues/14642 about the extra config options - let's keep this discussion about the attribute behavior and existing options.

I'm not sure this is the right order. I think the assembly attribute should win over SDK so a new assembly version does not require SDK update if there is a problem with the assembly.

I don't follow - could you clarify what kind of problem you mean? I think it makes sense for IsTrimmable=false metadata to override the attribute as an explicit opt-out. The SDK would not set this by default.

  • IsTrimmable msbuild metadata (true/false/not-present)

I'd propose making this just (present/not-present). Not-present means it will not be trimmed unless the developer explicitly opts in by setting IsTrimmable=true metadata.

  • TrimMode msbuild property (link/copy/copyused)
  • AssemblyTrimMode msbuild property (link/copy/copyused)

(There's no AssemblyTrimMode property - I think you mean TrimMode assembly metadata). These map directly to the command-line options -c/-u and -p. The plan is to set defaults using the attribute, so the SDK will no longer pass -p copy for everything outside netcoreapp, and TrimMode metadata will become unused out of the box. It could remain as an advanced option (similar to using the command-line), but I'd expect most people to use IsTrimmable and the global TrimMode instead.

Technically TrimMode can be set to any linker action, but I think it only really makes sense to use link/copyused. If we wanted to simplify this I think it would be ok to drop support for other actions.

@vitek-karas
Copy link
Member

My thinking when proposing the priority order was:

  • SDKs should only set IsTrimmable=true for libraries which didn't get the attribute yet. Ideally SDKs would not set any metadata and leave everything up to attributes.
  • The metadata was meant as a way for developers to override "defaults" - both ways. That's why setting the metadata always wins. SDK could also use it to "patch" a problem with an assembly (it's easier for us to service SDK than to service the assemblies). Also if some 3rd party assembly incorrectly uses the attribute the metadata would be able to override it.
  • per-assembly TrimMode should not be used by SDKs at all. Honestly I think we should get rid of it, since it allows the rather complex "mix mode", which I think is not important. But I'm fine keeping it as a way to basically directly influence the linker (this would win over everything else basically, just like it does today).

The easy to use properties (include all, and item group) are just that, make it easier. The main driver for that is the current complexity of modifying the defaults. It's just too complex. I understand we don't need to make it super easy, but we're currently making it very hard.

@marek-safar
Copy link
Contributor Author

I don't follow - could you clarify what kind of problem you mean? I think it makes sense for IsTrimmable=false metadata to override the attribute as an explicit opt-out. The SDK would not set this by default.

This to me is like brute force which can go wrong easily. SDK will probably not really deeply investigate the context of the assembly so let's use an example from Eric. Imagine we have System.IO.Ports assembly and someone hardcodes that into SDK as IsTrimmable=true, then we ship out of band release for a new platform which cannot be trimmed. What are we going to do, force everyone to pull new SDK service release and break them by not trimming?

The metadata was meant as a way for developers to override "defaults" - both ways. That's why setting the metadata always wins. SDK could also use it to "patch" a problem with an assembly (it's easier for us to service SDK than to service the assemblies).

Are we going to deal with RIDs and NuGet versions to make this actually work or is the idea to patch everyone everywhere regardless of implementation?

I wonder if we need the metadata IsTrimmable at all if we now have to send all assemblies to linker anyway for -p handling or attribute inspection.

There's no AssemblyTrimMode property - I think you mean TrimMode assembly metadata

That's what I understood as "Allow opting into trimming via an ItemGroup, not just via IsTrimmable metadata". I think it's crucial during the transition period to allow developers to easily experiment with enabling trimming for any assembly. It should be as simple as passing/adding msbuild property. Something like /p:AssemblyTrimModeLink=FSharp.Core (or whatever is best msbuild syntax). We could use the same for an explicit manual override for any assembly.

@sbomer
Copy link
Member

sbomer commented Nov 20, 2020

I wonder if we need the metadata IsTrimmable at all if we now have to send all assemblies to linker anyway for -p handling or attribute inspection.

I think it's crucial during the transition period to allow developers to easily experiment with enabling trimming for any assembly.

When I talk about IsTrimmable metadata, I'm talking about the project file knob that says "trim assembly X". Whether it's done via metadata or a new property, it should interact with the assembly attribute the same way. I added some more thoughts about using a property at https://github.com/dotnet/sdk/issues/14642#issuecomment-731460660.

Imagine we have System.IO.Ports assembly and someone hardcodes that into SDK as IsTrimmable=true, then we ship out of band release for a new platform which cannot be trimmed.

is the idea to patch everyone everywhere regardless of implementation?

I see what you mean - I would move away from hard-coding any IsTrimmable defaults into the SDK if we are going to use attributes. If we do it for servicing reasons, I'd be careful and do the patch very selectively (similar to how we currently have different IsTrimmable defaults for the .NET Core SDK and for the blazor SDK).

I think the IsTrimmable metadata is more useful for a developer who wants to override the attribute. The most important case is to let a developer trim an assembly that hasn't been attributed, but I could see the opt-out (IsTrimmable=false) being useful to work around linker bugs or incorrectly attributed assemblies.

@vitek-karas
Copy link
Member

Imagine we have System.IO.Ports assembly and someone hardcodes that into SDK as IsTrimmable=true, then we ship out of band release for a new platform which cannot be trimmed.

Ideally we would move away from specifying IsTrimmable=true in the SDK (it should never need to specify IsTrimmable=false unless we need to patch a problem) and instead use the attributes in assemblies. If we do screw up the attribute, then we could use SDK to patch that problem.

I wonder if we need the metadata IsTrimmable at all if we now have to send all assemblies to linker anyway

We're already sending all assemblies to the linker via --reference - in fact we've discussed some time ago that ideally linker would not do any probing when running from within the SDK - since it can get it wrong (SDK has a much more complex system to figure out the dependencies, letting linker sort of randomly search the disk is problematic).

I also don't understand why we need to read all assemblies to figure out the attribute. Maybe if the defaults are set to copy, for other defaults it should not be necessary. In the above proposal the attribute will not actually specify the link action on the assembly, it will only say if it should use the more conservative default, or the more aggressive action (I don't think linker has command line options to express this behavior today though) - and so linker can rely on the fact that the attribute may only make the assembly trim more aggressively.

@sbomer
Copy link
Member

sbomer commented Nov 23, 2020

why we need to read all assemblies to figure out the attribute. Maybe if the defaults are set to copy, for other defaults it should not be necessary

Assemblies without the attribute will need to be preserved, so we must look for the attribute to know if we are allowed to remove code from an assembly (or remove the whole assembly).

There are two "defaults":

  • Default behavior for trimmed assemblies (IsTrimmable attribute or metadata)

    This is what TrimMode does, currently by mapping to -c/-u.

  • Default behavior for non-trimmed assemblies (no IsTrimmable attribute/metadata)

    Currently we explicitly pass -p copy for non-IsTrimmable (metadata) assemblies, but this doesn't work for attributes (it would override them). We need new default behavior that will copy and root assemblies without the IsTrimmable attribute - either built in to the linker, or as another command-line flag (to avoid breaking the existing command-line behavior which doesn't respect the attribute).

@vitek-karas
Copy link
Member

I see - makes sense. We could probably do something super fast using S.R.Metadata and file mapping instead of Cecil just to get the attribute. We'll have to measure it a bit, but I don't think it's going to be too noticeable if done right.

sbomer added a commit to sbomer/linker that referenced this issue Nov 25, 2020
This includes some background on what we added for .NET5, and a proposal for more options in .NET6 :
- An assembly-level attribute to opt into trimming
- Simplified trimming opt-in from the SDK
- Simplified "trim all assemblies" flag

This is a summary of the discussion in dotnet#1269 and https://github.com/dotnet/sdk/issues/14642, and the .NET 5 discussion in dotnet/sdk#12035.
sbomer added a commit that referenced this issue Jan 4, 2021
* Add design doc for trimming opt-in/opt-out

This includes some background on what we added for .NET5, and a proposal for more options in .NET6 :
- An assembly-level attribute to opt into trimming
- Simplified trimming opt-in from the SDK
- Simplified "trim all assemblies" flag

This is a summary of the discussion in #1269 and https://github.com/dotnet/sdk/issues/14642, and the .NET 5 discussion in dotnet/sdk#12035.

* Add existing AssemblyMetadata example

* PR feedback

* Fix typo

* Update docs/design/trimmed-assemblies.md

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* Apply suggestions from code review

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* PR feedback

* Add notes about opt-out

* Use present tense

* Add more opt-out notes

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@vitek-karas
Copy link
Member

I think this has been basically solved via the IsTrimmable attribute. @sbomer is that the case?

@sbomer
Copy link
Member

sbomer commented Jun 17, 2021

I think so. Just to note differences from the original proposal:

The attribute[...] would indicate that during the assembly processing illinker didn't find any problems and are safe to be linked.

The linker would not add this attribute if added manually by the user.

The intent is for IsTrimmable to be used in an annotated linker-friendly assembly, but the tooling doesn't really enforce it - instead, you put <IsTrimmable>true</IsTrimmable> in the project file which both adds the attribute and turns on analysis warnings. But the tooling leaves the attribute there whether or not warnings are addressed.

The mode value would reflect the highest linker compatibility level and would make to linker actions

The IsTrimmable attribute doesn't have compatibility levels; it's either there or it's not.

@sbomer sbomer closed this as completed Jun 17, 2021
agocke pushed a commit to dotnet/runtime that referenced this issue Nov 16, 2022
* Add design doc for trimming opt-in/opt-out

This includes some background on what we added for .NET5, and a proposal for more options in .NET6 :
- An assembly-level attribute to opt into trimming
- Simplified trimming opt-in from the SDK
- Simplified "trim all assemblies" flag

This is a summary of the discussion in dotnet/linker#1269 and https://github.com/dotnet/sdk/issues/14642, and the .NET 5 discussion in dotnet/sdk#12035.

* Add existing AssemblyMetadata example

* PR feedback

* Fix typo

* Update docs/design/trimmed-assemblies.md

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* Apply suggestions from code review

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

* PR feedback

* Add notes about opt-out

* Use present tense

* Add more opt-out notes

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Commit migrated from dotnet/linker@c45a25d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants