Skip to content

Generate custom attributes from metadata instead of type system #100763

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.

Sign up for GitHub

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

Merged
MichalStrehovsky merged 8 commits into from
Apr 16, 2024
Merged

Generate custom attributes from metadata instead of type system #100763

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6ed48bc
wip
MichalStrehovsky Apr 8, 2024
de5ce9a
Update Transform.CustomAttribute.cs
MichalStrehovsky Apr 8, 2024
2866dac
Fixes
MichalStrehovsky Apr 9, 2024
1892ab3
Update Transform.CustomAttribute.cs
MichalStrehovsky Apr 9, 2024
7d5e6bb
Rename
MichalStrehovsky Apr 15, 2024
14b76b3
Test update
MichalStrehovsky Apr 15, 2024
338a688
UnreachableException
MichalStrehovsky Apr 15, 2024
2bbdc90
Assert -> Throw
MichalStrehovsky Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update Transform.CustomAttribute.cs
  • Loading branch information
@MichalStrehovsky
MichalStrehovsky committed Apr 9, 2024
commit 1892ab34cad179a3c3c069c7b48ce936e24ef372
7 changes: 6 additions & 1 deletion ...r/tools/aot/ILCompiler.MetadataTransform/ILCompiler/Metadata/Transform.CustomAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ private MetadataRecord HandleCustomAttributeConstantValue(Cts.Ecma.EcmaModule mo
return s == null ? new ConstantReferenceValue() : HandleString(s);
}

if (typeCode == Ecma.SerializationTypeCode.Type)
{
string s = valueReader.ReadSerializedString();
return s == null ? new ConstantReferenceValue() : HandleType(Cts.CustomAttributeTypeNameParser.GetTypeByCustomAttributeTypeName(module, s));
}

return typeCode switch
{
Ecma.SerializationTypeCode.Boolean => new ConstantBooleanValue { Value = valueReader.ReadBoolean() },
Expand All @@ -159,7 +165,6 @@ private MetadataRecord HandleCustomAttributeConstantValue(Cts.Ecma.EcmaModule mo
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),
Ecma.SerializationTypeCode.Type => HandleType(Cts.CustomAttributeTypeNameParser.GetTypeByCustomAttributeTypeName(module, valueReader.ReadSerializedString())),
_ => throw new System.Exception()
Copy link
Member

@jkotas jkotas Apr 14, 2024

Choose a reason for hiding this comment

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

Cts.ThrowHelper.ThrowBadImageFormatException or throw new UnreachableException() ?

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Apr 15, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jkotas jkotas Apr 15, 2024

Choose a reason for hiding this comment

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

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().

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Apr 16, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jkotas jkotas Apr 17, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Apr 17, 2024

Choose a reason for hiding this comment

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

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.

};
}
Expand Down