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

[x86/linux] Fix redefined DISPATCHER_CONTEXT compile error#8246

Merged
janvorli merged 1 commit into
dotnet:masterfrom
seanshpark:x86pal02
Nov 23, 2016
Merged

[x86/linux] Fix redefined DISPATCHER_CONTEXT compile error#8246
janvorli merged 1 commit into
dotnet:masterfrom
seanshpark:x86pal02

Conversation

@seanshpark
Copy link
Copy Markdown

@seanshpark seanshpark commented Nov 22, 2016

WIP, fix compile error for x86/Linux

  • add directive !FEATURE_PAL to current DISPATCHER_CONTEXT in clrnt.h
  • add DISPATCHER_CONTEXT for x86 in palrt.h

@seanshpark
Copy link
Copy Markdown
Author

@janvorli , @jkotas , @gkhanna79 , PTAL

Copy link
Copy Markdown
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Comment thread src/inc/clrnt.h Outdated
//

#if defined(_TARGET_X86_)
#if defined(_TARGET_X86_) && defined(WIN32)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be "defined(_MSC_VER)" to only enable these pragmas when compiling with VC++ toolset and not just win32 (which maybe true for LLVM based compile, for example)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This ifdef block doesn't contain just the warning pragma, but also the _DISPATCHER_CONTEXT.
But thinking about it more, I think we should use !defined(FEATURE_PAL) instead, since the other place where the _DISPATCHER_CONTEXT is defined for x86 Linux is included for FEATURE_PAL only.
Btw, it seems the warning disabling pragmas are outdated, there is no function definition between the pragmas.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good to me.

it seems the warning disabling pragmas are outdated, there is no function definition between the pragmas.

In this case, @seanshpark can you please remove the pragmas as well?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, I'll change to !defined(FEATURE_PAL) and remove pragmas

@seanshpark
Copy link
Copy Markdown
Author

#define PcTeb 0x18 is also removed as it is not used anywhere.

WIP, fix compile error for x86/Linux
- add directive WIN32 to current DISPATCHER_CONTEXT in clrnt.h
- add DISPATCHER_CONTEXT for x86 in palrt.h
@seanshpark
Copy link
Copy Markdown
Author

@dotnet-bot test Windows_NT x64 Debug Build and Test please
@dotnet-bot test Windows_NT x86 legacy_backend Checked Build and Test please

@seanshpark
Copy link
Copy Markdown
Author

done changing to !defined(FEATURE_PAL)

Copy link
Copy Markdown
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit 2c67c72 into dotnet:master Nov 23, 2016
@seanshpark seanshpark deleted the x86pal02 branch November 23, 2016 22:41
sergign60 pushed a commit to sergign60/coreclr that referenced this pull request Dec 2, 2016
WIP, fix compile error for x86/Linux
- add directive WIN32 to current DISPATCHER_CONTEXT in clrnt.h
- add DISPATCHER_CONTEXT for x86 in palrt.h
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…reclr#8246)

WIP, fix compile error for x86/Linux
- add directive WIN32 to current DISPATCHER_CONTEXT in clrnt.h
- add DISPATCHER_CONTEXT for x86 in palrt.h

Commit migrated from dotnet/coreclr@2c67c72
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.

5 participants