Generate custom attributes from metadata instead of type system #100763
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dotnet/ilc-contrib could someone have a look? This is switching custom attribute metadata generation from The changes to |
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/Generator/SchemaDef.cs
Outdated
Show resolved
Hide resolved
...eclr/tools/aot/ILCompiler.MetadataTransform/ILCompiler/Metadata/Transform.CustomAttribute.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/Generator/SchemaDef.cs
Outdated
Show resolved
Hide resolved
| Ecma.SerializationTypeCode.Single => new ConstantSingleValue { Value = valueReader.ReadSingle() }, | ||
| Ecma.SerializationTypeCode.Double => new ConstantDoubleValue { Value = valueReader.ReadDouble() }, | ||
| Ecma.SerializationTypeCode.SZArray => HandleCustomAttributeConstantArray(module, valueReader.ReadSerializationTypeCode(), ref valueReader), | ||
| _ => throw new System.Exception() |
There was a problem hiding this comment.
Cts.ThrowHelper.ThrowBadImageFormatException or throw new UnreachableException() ?
There was a problem hiding this comment.
Cts.ThrowHelper.ThrowBadImageFormatException is not a throw for C#, so it won't work.
Any opinion about simply disabling warning CS8509 ILCompiler-project-wide?
We keep having to work around it by adding artificial throws, or worse, by adding _ => for something that could be one concrete value (SomeFoo =>), but the right-hand side happens to throw for all the other possibilities, so it's as good as an extra _ => throw new Blah() line.
There was a problem hiding this comment.
adding _ => for something that could be one concrete value (SomeFoo =>)
The compiler is going to add throw for cases that are not covered by the switch. The difference here is whether the code is explicitly handling it with a throw or whether you let the compiler to generate the throw implicitly. Many of the compiler warnings and our own coding conventions rules force you to write more verbose and explicit code, so I see the CS8509 warning as just another one of those.
I think we should throw BIF for invalid input files when we have a choice. If these default switch cases are reachable for invalid input (it is not obvious to me whether it is the case), we may want to add Cts.ThrowHelper.CreateBadImageFormatException and change them to throw Cts.ThrowHelper.CreateBadImageFormatException().
There was a problem hiding this comment.
The thing is that I would be fine with the SwitchExpressionException the compiler throws. Anything that kills the compilation is fine by me.
I agree that our coding standards err on the more verbose side, so throw new UnrechableException sounds like the best fix.
Throwing a Cts.BadImageException wouldn't make a difference here. This spot is non-recoverable; we're in the middle of generating metadata and can't change our mind about it. We already inspected the attribute for dependencies during dependency analysis so at this point we know it doesn't refer to something that doesn't exist (we'd skip it if it does). If it has some other structural issue we didn't detect during dependency analysis, I'm fine with a compiler crash. We try to be resilient for invalid inputs that could be the result of bad assembly version management, but we don't try to be resilient against source-to-IL compiler bugs. Too much resilience makes it difficult to surface our own bugs because this would also paper over those.
There was a problem hiding this comment.
We try to be resilient for invalid inputs that could be the result of bad assembly version management,
Parsing of custom attribute blobs can hit BadImageFormat exception due to user error. If the custom attribute references enum from a different assembly and the enum underlying type changes due to user error, parsing of the custom attribute blob will get lost and it will likely end up hitting one of these unreachable cases.
There was a problem hiding this comment.
That's a good point but it still needs to go to the other spot that parses it first (at the time we analyze dependencies). #101164 has the fix.
| Ecma.SerializationTypeCode.UInt64 => new ConstantUInt64Array { Value = GetCustomAttributeConstantArrayElements<ulong>(ref valueReader, count) }, | ||
| Ecma.SerializationTypeCode.Single => new ConstantSingleArray { Value = GetCustomAttributeConstantArrayElements<float>(ref valueReader, count) }, | ||
| Ecma.SerializationTypeCode.Double => new ConstantDoubleArray { Value = GetCustomAttributeConstantArrayElements<double>(ref valueReader, count) }, | ||
| _ => throw new System.Exception() |
src/libraries/System.Runtime/tests/System.Reflection.Tests/CustomAttributeTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Reflection.Tests/CustomAttributeTests.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Reflection.Tests/CustomAttributeTests.cs
Outdated
Show resolved
Hide resolved
…et#100763) Fixes dotnet#100688 Fixes a bit more than just what was reported (see the test). We were losing type information in `attribute.DecodeValue` and could no longer distinguish between `new object[] { SomeEnum.Val }` and `new SomeEnum[] { SomeEnum.Val }`. The fix required a complete rewrite of attribute emission using the more low level API. Cc @dotnet/ilc-contrib
Fixes #100688
Fixes a bit more than just what was reported (see the test). We were losing type information in
attribute.DecodeValueand could no longer distinguish betweennew object[] { SomeEnum.Val }andnew SomeEnum[] { SomeEnum.Val }.The fix required a complete rewrite of attribute emission using the more low level API.
Cc @dotnet/ilc-contrib