Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Initialize Stack Allocated Data Structures in TraceLogging #16259

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

brianrob
Copy link
Member

@brianrob brianrob commented Feb 7, 2018

TraceLogging was broken by #13728 because data structures allocated with stackalloc within code in System.Private.CoreLib are no longer zero-initialized.

TraceLogging ETW tests are not currently enabled and thus, there was no test coverage for this case. I found this break while working to enable the TraceLogging ETW tests.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it!

@stephentoub
Copy link
Member

I see that EventData is three fields:

internal ulong m_Ptr;
internal int m_Size;
#pragma warning disable 0649
internal int m_Reserved; // Used to pad the size to match the Win32 API
#pragma warning restore 0649

and in pretty much all of our other providers, we set only DataPointer and Size, and don't zero-init Reserved. I don't know if the native implementation relies on those being zero. Do we need to go through our other EventSources and zero-out as well?

@brianrob
Copy link
Member Author

brianrob commented Feb 7, 2018

@stephentoub TraceLogging actually uses Reserved and so I believe that is the cause of this break. From what I can see manifest-based ETW does not and so that's likely why manifest-based EventSources are unaffected. We could do this, but I'm not sure that we need to as requiring this would likely be a significant breaking change on the ETW native APIs. @vancem, do you know if Reserved must be zero'd?

@vancem
Copy link

vancem commented Feb 7, 2018

@brianrob. While I could well believe that old ETW code does nothing with the 'Reserved' field, but it really needs to be set to 0 by the clients (that is us) for there to be ANY chance of a Reserved field serving its purpose (which is to allow additional information to be passed in later versions). If clients put trash in there we can never distinguish between trash and a legitimate new use for that field.

Thus I would consider it a bug if we did not initialize Reserved to 0.

@brianrob
Copy link
Member Author

brianrob commented Feb 7, 2018

Ok, thanks @vancem. I will address this in a subsequent PR.

@brianrob brianrob merged commit f2d4300 into dotnet:master Feb 7, 2018
@brianrob brianrob deleted the fix_tracelogging branch February 7, 2018 22:47
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Feb 7, 2018
Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
stephentoub pushed a commit to dotnet/corefx that referenced this pull request Feb 8, 2018
…26943)

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Feb 11, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Feb 11, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Feb 12, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants