Explictly initialize EventData.Reserved for System.Private.CoreLib#16283
Explictly initialize EventData.Reserved for System.Private.CoreLib#16283brianrob merged 1 commit intodotnet:masterfrom
Conversation
|
Run EventSource tests: @dotnet-bot test Windows_NT x64 Checked corefx_baseline |
| /// Reserved by ETW. This property is present to ensure that we can zero it | ||
| /// since System.Private.CoreLib uses are not zero'd. | ||
| /// </summary> | ||
| internal int Reserved { get { return m_Reserved; } set { m_Reserved = value; } } |
There was a problem hiding this comment.
What will the recommended pattern be for code outside of corelib that decides to enable clearing of initlocals?
descrs[0] = new EventData
{
DataPointer = ...,
Size = ...
};instead of:
descrs[0].DataPointer = ...;
descrs[0].Size = ...;?
There was a problem hiding this comment.
I was looking at wrong copy of EventData
There was a problem hiding this comment.
That's a good question. I was not aware of this effort until recently, and did not know that this is something that will go beyond CoreLib. Can you tell me more about how this will work or point me to something that I can read?
It's possible to pre-initialize the structure prior to using it, but that might be a big hammer:
EventData *pData = stackalloc EventData[4];
for(int i=0; i<4; i++)
{
pData[i] = default(EventData);
}
We can also consider adding a public property to EventData, but I feel like I need to understand more about the overall goal of allowing people to clear preinitlocals and what if anything else besides stackalloc is affected by this flag.
There was a problem hiding this comment.
It took me a minute to catch on to your recommendation above. Your proposal would also work:
descrs[0] = new EventData
{
DataPointer = ...,
Size = ...
};
There was a problem hiding this comment.
Can you tell me more about how this will work or point me to something that I can read?
There's an ILLink step that can clear the flag:
https://github.com/mono/linker/blob/86b09e1033d68d4cc1aaa2c7362c16b756d6cbf3/corebuild/integration/ILLink.CustomSteps/ClearInitLocals.cs
https://github.com/dotnet/corefx/pull/25956/files#diff-c67ad2f29572959e709529bded55461aR75
There's also a proposal to support this directly in Roslyn:
https://github.com/dotnet/csharplang/blob/master/proposals/skip-localsinit.md
The latter link has a good description of the issues and affect.
There was a problem hiding this comment.
Thanks @stephentoub. I will follow-up on this to see if we want to do anything more than:
descrs[0] = new EventData
{
DataPointer = ...,
Size = ...
};
There was a problem hiding this comment.
@stephentoub' suggestion works (although I have not looked at its codegen, it may be good).
I debated making DataPointer clear the Reserved field, but that penalizes the common case for the sake of the uncommon case.
I think that advice is fine.
|
@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test |
|
System.Diagnostics.Tracing.Tests passed. The CoreFx failures appear to be due to a File I/O bug. |
In System.Private.CoreLib stackalloc allocations are no longer zeroed. This change ensures that EventData.Reserved is always initialized to zero before it is passed to ETW.
This initialization is done early enough that EventSource can change the value of Reserved if required by TraceLogging.
In addition to CI validation, I am also doing some validation with these EventSources enabled.