Skip to content
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.

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

Develop/issue 485 #486

Merged
merged 3 commits into from
Feb 5, 2018
Merged

Develop/issue 485 #486

merged 3 commits into from
Feb 5, 2018

Conversation

SteveGilham
Copy link
Contributor

This is as much a straw-man proposal as anything, exposing the linker version as a ushort, rather than trying to assembly a full history of possible linker versions as an enum (C++/CLI uses non-zero minor versions -- currently VS2017 v15.5.5 gives 14.12 -- and I'm not sure I can catch them all).

Watching in the debugger while the unit tests ran, we have everything from linker version 6 (a very early .net version indeed) through to 80 (VB Roslyn) coming through to the image writer, so the changes have been put through their paces.

@SteveGilham
Copy link
Contributor Author

The Travis job seems inherently borked, as it didn't even start building.

@@ -333,7 +336,12 @@ public sealed class ModuleDefinition : ModuleReference, ICustomAttributeProvider
set { characteristics = value; }
}

[Obsolete("Use FileName")]
public ushort Linker {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need to expose this for now, I suggest we handle this like the timestamp.

@@ -260,6 +262,7 @@ public sealed class ModuleDefinition : ModuleReference, ICustomAttributeProvider
TargetArchitecture architecture;
ModuleAttributes attributes;
ModuleCharacteristics characteristics;
ushort linker;
Copy link
Owner

@jbevain jbevain Feb 1, 2018

Choose a reason for hiding this comment

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

internal ushort linker_version = 8;

@@ -240,6 +240,8 @@ public sealed class WriterParameters {

public sealed class ModuleDefinition : ModuleReference, ICustomAttributeProvider, ICustomDebugInformationProvider, IDisposable {

private static ushort defaultLinkerValue = 0; // or 8 to retain previous default behaviour
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can remove this safely in favor of using 8 in the declaration, see below.

WriteByte (8); // LMajor
WriteByte (0); // LMinor
WriteUInt16 ((ushort) (!pe64 ? 0x10b : 0x20b)); // Magic
WriteUInt16 (module.Linker);
Copy link
Owner

Choose a reason for hiding this comment

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

module.linker_version

@@ -81,15 +81,16 @@ void ReadImage ()
// Characteristics 2
ushort characteristics = ReadUInt16 ();

ushort subsystem, dll_characteristics;
ReadOptionalHeaders (out subsystem, out dll_characteristics);
ushort subsystem, dll_characteristics, linker;
Copy link
Owner

Choose a reason for hiding this comment

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

please standardize on linker_version

@@ -58,6 +59,7 @@ sealed class Image : IDisposable {
public Image ()
{
counter = GetTableLength;
Linker = 0;
Copy link
Owner

@jbevain jbevain Feb 1, 2018

Choose a reason for hiding this comment

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

No need to initialize it.

@@ -28,6 +28,7 @@ sealed class Image : IDisposable {
public string RuntimeVersion;
public TargetArchitecture Architecture;
public ModuleCharacteristics Characteristics;
public ushort Linker;
Copy link
Owner

Choose a reason for hiding this comment

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

LinkerVersion here too :)

@jbevain
Copy link
Owner

jbevain commented Feb 1, 2018

Hello Steve,

Don't worry about Travis, it has its up and down.

I've added a bunch of comments, please address them, and then this will be good to go.

Thanks for your contribution!

@SteveGilham
Copy link
Contributor Author

Thanks for the review.

With this change in place I shall be able to revert to using master and just reflectively tweak my F# binaries to look like they came from the C# Roslyn compiler.

@jbevain jbevain merged commit efa6341 into jbevain:master Feb 5, 2018
@jbevain
Copy link
Owner

jbevain commented Feb 5, 2018

Thanks for your contribution to Cecil!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants