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

Explictly initialize EventData.Reserved for System.Private.CoreLib#16283

Merged
brianrob merged 1 commit intodotnet:masterfrom
brianrob:eventsource_initialization
Feb 8, 2018
Merged

Explictly initialize EventData.Reserved for System.Private.CoreLib#16283
brianrob merged 1 commit intodotnet:masterfrom
brianrob:eventsource_initialization

Conversation

@brianrob
Copy link
Member

@brianrob brianrob commented Feb 8, 2018

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.

@brianrob
Copy link
Member Author

brianrob commented Feb 8, 2018

Run EventSource tests: @dotnet-bot test Windows_NT x64 Checked corefx_baseline

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!

/// 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; } }
Copy link
Member

Choose a reason for hiding this comment

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

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 = ...;

?

Copy link
Member

@jkotas jkotas Feb 8, 2018

Choose a reason for hiding this comment

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


I was looking at wrong copy of EventData

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

It took me a minute to catch on to your recommendation above. Your proposal would also work:

descrs[0] = new EventData
{
    DataPointer = ...,
    Size = ...
};

Copy link
Member

@stephentoub stephentoub Feb 8, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @stephentoub. I will follow-up on this to see if we want to do anything more than:

descrs[0] = new EventData
{
    DataPointer = ...,
    Size = ...
};

Copy link

Choose a reason for hiding this comment

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

@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.

@brianrob
Copy link
Member Author

brianrob commented Feb 8, 2018

@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test

@brianrob
Copy link
Member Author

brianrob commented Feb 8, 2018

System.Diagnostics.Tracing.Tests passed. The CoreFx failures appear to be due to a File I/O bug.

@brianrob brianrob merged commit 7b204f2 into dotnet:master Feb 8, 2018
@brianrob brianrob deleted the eventsource_initialization branch February 8, 2018 21:24
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