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

License #87

Closed
SelvinPL opened this issue Nov 13, 2018 · 8 comments
Closed

License #87

SelvinPL opened this issue Nov 13, 2018 · 8 comments

Comments

@SelvinPL
Copy link

SelvinPL commented Nov 13, 2018

In original code there is lack of license to original code (published in 2006 with CPOL license)
You can see that that was a base code in state's names and state's values like addilne, classnames

@3F
Copy link
Owner

3F commented Nov 13, 2018

@SelvinPL How does it related to our project?

About origin - #3

@3F 3F added the unclear label Nov 13, 2018
@SelvinPL
Copy link
Author

SelvinPL commented Nov 13, 2018

How does it related to our project? since you are using Robert Giesecke's code and in the fact that Robert Giesecke didn't mention that his code was based on my code(see the il parser code) which was under CPOL license which is not compatible with MIT

anyway, the only point that i wrote this is the point 5a and 5e of the license "You agree not to remove any of the original copyright" which was "Copyright © Osadkowski S.A. 2006"

@den-run-ai
Copy link

Any idea @RobertGiesecke?

@SelvinPL
Copy link
Author

SelvinPL commented Nov 13, 2018

anyway I've change license of article to MIT

example of code article

    case ParserState.ClassDeclaration:
	if (trimedline.StartsWith("{"))
	{
	    state = ParserState.Class;
	    string classname = "";
	    System.Text.RegularExpressions.Regex reg = new System.Text.RegularExpressions.Regex(@".+\s+([^\s]+) extends \[.*");
	    System.Text.RegularExpressions.Match m = reg.Match(classdeclaration);
	    if (m.Groups.Count > 1)
		classname = m.Groups[1].Value;
	    classname = classname.Replace("'", "");
	    if (classnames.Count > 0)
		classname = classnames.Peek() + "+" + classname;
	    classnames.Push(classname);
	    Console.WriteLine("Found class: " + classname);
	    wholeilfile.Add(classdeclaration);
	}
	else
	{
	    classdeclaration += " " + trimedline;
	    addilne = false;
	}
	break;

version 1.2.1.28776 DllExport

[ParserStateAction(ParserState.ClassDeclaration)]
public sealed class ClassDeclarationParserAction : IlParser.ParserStateAction
{
	public override void Execute(ParserStateValues state, string trimmedLine)
	{
		if (trimmedLine.StartsWith("{"))
		{
			state.State = ParserState.Class;
			string text = "";
			Match match = state.MatchClass(state.ClassDeclaration);
			if (match.Groups.Count > 1)
			{
				text = match.Groups[1].Value.NullSafeCall((string v) => v.Replace("'", ""));
			}
			if (state.ClassNames.Count > 0)
			{
				text = state.ClassNames.Peek() + "+" + text;
			}
			state.ClassNames.Push(text);
			state.Result.Add(state.ClassDeclaration);
		}
		else
		{
			state.ClassDeclaration = state.ClassDeclaration + " " + trimmedLine;
			state.AddLine = false;
		}
	}
}

@3F 3F added origin news and removed unclear labels Nov 13, 2018
@3F
Copy link
Owner

3F commented Nov 13, 2018

Hmm, I've checked both of your direction in logic for parsing the IL code, so partially it seems to be true.

It more like he was at least inspired by the your idea of your parser. But, the current source code (this repo continues 1.2.7), has a very weak relationship with your code, for example:

You have this adding:

if (trimedline.StartsWith(".corflags"))
{
    wholeilfile.Add(".corflags 0x00000002");
    wholeilfile.Add(string.Format(".vtfixup [{0}] int32 fromunmanaged at VT_01", exportscount));
    wholeilfile.Add(string.Format(".data VT_01 = int32[{0}]", exportscount));
    Console.WriteLine("Adding vtfixup.");
    addilne = false;
}

It seems it was necessary to explicitly set the runtime header flags in ILAsm v1.x (an manual adding .corflags + .vtfixup -> .data) but our IL parser works only with 2.0+ So we have no this at all.

While the addilne flag is the most used common logic, you do not have a single approach:

switch (state)
{
    case ParserState.Normal:
        if (trimedline.StartsWith(".corflags"))
        {
            ...
            addilne = false;
        }
        else if (trimedline.StartsWith(".class"))
        {
            ...
            addilne = false;
        }
        else if (trimedline.StartsWith(".assembly extern ExportDllAttribute"))
        {
            addilne = false;
            ...
        }

Robert's logic looks like:

public override void Execute(ParserStateValues state, string trimmedLine)
{
    if(trimmedLine.StartsWith(".class", StringComparison.Ordinal))
    {
        ...
        state.AddLine = true;
    }
    else if(trimmedLine.StartsWith(".method", StringComparison.Ordinal))
    {
        ...
        state.AddLine = false;
    }
....

public override void Execute(ParserStateValues state, string trimmedLine)
{
    if(trimmedLine.StartsWith(".corflags", StringComparison.Ordinal))
    {
        ...
        state.AddLine = false;
    }
    else if(trimmedLine.StartsWith(".class", StringComparison.Ordinal))
    {
        ...
        state.AddLine = true;
    }

By the way, looks like you will have a similar to #59 bug for:

case ParserState.Method:
   if (trimedline.StartsWith("} // end of method")) 

Even your parsing in ParserState.MethodDeclaration is not the same at all. You have trivial regex ?<before>[^\(]+\s+)(?<method>[^\(]+)(?<after>\(.*) we have complex manual stepping for different case.

Your mentioned ClassDeclarationParserAction from v1.2.7 contains own iteration. But for stack-based logic it's normal to push found elements before new deepening.

The only one part from 1.2.7 that really looks like this is you, the following ParserState enum:

enum ParserState
{
    Normal,
    ClassDeclaration,
    Class,
    DeleteExportDependency,
    MethodDeclaration,
    MethodProperties,
    Method,
    DeleteExportAttribute,
}

So I need to hear Robert's story, or I need detailed code matching.

Please continue to compare 1.2.7 version, because more probably Robert was trying to avoid dependencies with you if it was (seems it was) in earlier versions. Thus, he could completely rewrite initial logic. Or he was just inspired by your idea. I don't know. But anyway we don't follow any versions less than 1.2.7.

Mainly I agree, the parsing logic looks similar in direction like you but StartsWith/addLine/stack-based iteration is a common way for understanding. And most important, we do not set the runtime header flags at all, not even .vtfixup, and also methods parsing is not similar at all.

tl;dr I'm not against to add you in Licence, but I don't know the whole story about this. Please understand, I'll try to solve your issue soon as possible. Briefly, as I explained above, I can confirm only ParserState enum without logic for 1.2.7.

So please everyone, clarify this as possible. I'll try to contact directly with Robert if he doesn't answer here.

@RobertGiesecke
Copy link

RobertGiesecke commented Nov 14, 2018

Hi,
this was a long time ago...
I did remember seeing his article when I already had a working prototype (but his article might predate my work, not sure).
I started with it, because too many times, folks would write that this kind of thing would be impossible.

I remember that some things were eerily similar. I also used a state machine and my enum was almost the same.
Even that eye sore "} // end of class" was in your code as well. I think, I saw someone else at that time searching for that string as well. (You were probably just as afraid to read over a class boundary than I was)
The attribute had to be the same. We can only change the export name and the convention, everything else is just the result of mirroring DllImportAttribute.

However, if I remember correctly, I already used Cecil in my first prototypes to get the attributes and their values reliably.
I must have liked some of the things I saw in your code, so that over time things in mine started to follow some of your ideas.

I am absolutely unsure (and curse myself for not using a VCS back then), whether I followed your approach to get rid of the attribute dependency or not.
I think, I just didn’t care when it was still a project template. But when it became a nuget packages, I had to provide an assembly with that attribute, and I didn’t want to create a runtime dependency.
But even if I already did that or did it later on, I probably recognized and skipped those lines according to my copy of Expert .NET 2.0 IL Assembler

The obvious pieces like naming a variable „trimmedLine“ are no indication for a copy, I used that name so many a hundred times for a trimmed line, as did probably everybody else.
I don’t recall copying anything from your code, but looking at it now, I have to say I that some things got closer to yours over time.

And btw, this repo here is not my code. He used ilspy or reflector to get C# code from my binaries.
And wasn’t the bulk of the work getting the correct locations and versions of il(d)asm and dealing with Microsoft’s constant campaign of breaking how to find those things. Which is why I stopped caring about it a long time ago.

@SelvinPL
Copy link
Author

Fair enough, please close the issue

@3F
Copy link
Owner

3F commented Nov 14, 2018

dealing with Microsoft’s constant campaign of breaking how to find those things

:) lot of things. There was many products from 2015-2017 with breaking of backward compatibility for paths and features in that period.

Also some hard-coded moments like when I was starting distribute ILAsm on coreclr for this project that required external converter of resources .res -> obj COFF-format (to place in .rsrc section - PE). So now there were a number of my new private/open things and many discussions with MS to avoid these incomprehensible actions and <_<

Fair enough, please close the issue

Okay, it seems the issue has been resolved. Closed.

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

No branches or pull requests

4 participants