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

"Fix" sym_resolve in python bindings #273

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wsxarcher
Copy link
Contributor

# is this the missing symbol we want to handle?
if symbol == "_l1":
# put value of this symbol in @value
value = 0x1002
p_value.contents.value = 0x1002
Copy link
Member

@aquynh aquynh Jan 6, 2017

Choose a reason for hiding this comment

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

oh now i see that this interface does not look good. would be simpler for user to return the value, rather than set the content value like this.

the reason is that this resolver must return bool value to indicate if it sets the symbol. perhaps we should change the API a bit: this function either returns symbol value, or None when it does not care. so the function above will be changed to be:

    def sym_resolver(symbol):
        # is this the missing symbol we want to handle?
        if symbol == "_l1":
            # we handled this symbol, so return symbol value
            return 0x1002

        # we did not handle this symbol, so return None
        return None

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks simpler to the end user but the C API should be changed... And what about dealing with the symbols resolved 0 in the C? Another problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be a problem if the bindings must respect the C API, but of course it can differ.

Copy link
Member

Choose a reason for hiding this comment

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

we need a wrapper for this.

btw, with this PR applied, suite/regress/x64_sym_resolver.py still does not pass yet, right?

Copy link
Contributor Author

@wsxarcher wsxarcher Jan 6, 2017

Choose a reason for hiding this comment

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

You can also use [] but maybe it's less explicit I think

def sym_resolver(symbol, value):
        # is this the missing symbol we want to handle?
        if symbol == "_l1":
            # we handled this symbol, so return symbol value
            value[0] = 0x1002
            return True

        # we did not handle this symbol, so return None
        return None

Yes, x64_sym_resolver.py still not pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix does not work at all.
sample.py works because jump relative symbol are not resolved by sym_resolve, so it is not called in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(because in that case is calculated by the starting point + instruction length)

Copy link
Member

Choose a reason for hiding this comment

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

i just fixed Python binding in the "test" branch, so sample.py works now. but your x86_call_ptr_sym.py still fails for some unknown reason ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe call is resolved differently from jmp? What about suite/regress/x64_sym_resolver.py ?

Copy link
Member

Choose a reason for hiding this comment

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

there is a bug in the core for that testcase. will see how to fix that.

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