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

Current API for object loading is inadequate #230

Closed
edsko opened this issue Jul 31, 2014 · 15 comments
Closed

Current API for object loading is inadequate #230

edsko opened this issue Jul 31, 2014 · 15 comments

Comments

@edsko
Copy link
Collaborator

edsko commented Jul 31, 2014

Now that we have moved to dynamic-by-default, object files need to be resolved after loading -- this was the underlying cause for #228 (actually, that sentence is somewhat speculative). Currently we now call resolveObjs every time we load an object, but this is incorrect in the case that we want to load multiple object files which refer to each other. This may or may not matter (@snoyberg?).

edsko added a commit that referenced this issue Jul 31, 2014
This closes #228, but exposes a problem with the current API; opened a separate
ticket about that (#230).
@snoyberg
Copy link
Member

I need to understand three things a bit better:

  1. What's the trigger of this bug? Does it only apply when we have two C files that each use a symbol defined in the other C file?
  2. What would a user experience when triggering this bug? Just a non-responsive backend without any error message?
  3. How long do you estimate it will take to fix this?

@edsko
Copy link
Collaborator Author

edsko commented Jul 31, 2014

  1. It doesn't even have to be "each". As soon as one object file A refers to a symbol in another object file B, and we happen to load A before B, then resolving A's symbols will fail.
  2. Currently, yes (also because of the client library does not detect when child server (for snippet execution) segfaults #229)
  3. A fix is not particularly difficult -- we just have to call loadObj on the object files before calling resolveObjs when they have all been loaded. We just have to make a policy decision on how this translates to an ide-backend API.

@snoyberg
Copy link
Member

I would have thought this would be completely internal to ide-backend, and not affect the external API at all. What kind of API change would you picture coming from fixing this?

And based on your answer to (2), I would like to move ahead with fixing this.

@edsko
Copy link
Collaborator Author

edsko commented Jul 31, 2014

Well ide-backend needs to have a point where it can decide "ok, now we have all the object files we need so we can resolve them now". If this is entirely internal, then at which point do we decide that? I guess if it's to be entirely internal then the only reasonable choice is "resolve at (the end of) each call to updateSession" -- but that means that loading object file A in one call to updateSession and object file B in another will not work -- all related object files (or rather, .c files) must then be added in a single session update.

@snoyberg
Copy link
Member

I think I don't understand the problem well enough. From our side as the caller, we know nothing about object files. We will simply be providing source files ending in .c, and leave it up to ide-backend to determine that those are turning into object files. Furthermore, we'll have no control of when users will add or remove .c files from the project, they may appear at any time. So I think we have no way of providing you with the information you're looking for.

@edsko
Copy link
Collaborator Author

edsko commented Jul 31, 2014

I see, fair enough. So what you would prefer to happen, ideal case, if a user adds a C file that refers to an as-yet undefined symbol? And then subsequently adds a further C file that defines that symbol?

@snoyberg
Copy link
Member

Ideally:

  1. Give an error message "symbol X is undefined"
  2. Just start working

@edsko
Copy link
Collaborator Author

edsko commented Jul 31, 2014

Haha, ok :) So I guess that means we need to try to resolve object files whenever they are added, when that fails unload them again, and then try again when we have more object files. Also, come to think of it, if one C file changes we probably have to unload all object files in case any of the other object files refers to that particular object file. We will need to keep a bit more state ide-backend-side to be able to do all that.

@dcoutts
Copy link
Collaborator

dcoutts commented Jul 31, 2014

Yes, I think from a UI pov updateSession should correspond to a compile and link check, so if there are any dangling C symbol references then they should be reported as compile errors in the IDE. So yes, I think we do need to resolveObjs in updateSession if any object files have changed.

edsko added a commit that referenced this issue Aug 18, 2014
Although this all seems to work just fine, it _something_ does go wrong with
relocation, because when I add one more printf statement in defined_in_B things
break. I don't yet know why.
@edsko
Copy link
Collaborator Author

edsko commented Aug 18, 2014

So I started looking at this in detail. I have a test which loads two C object files A and B, where B calls into A, and a Haskell file which calls into the second C file. What I'm testing is what happens when I load these in various orders.

If I load first A by itself, then B, then the Haskelll file works. Of course, that's the easy case because at every step all dependencies are already available.

But if I first load B, then A, then the Haskell file, it doesn't work. I thought it did initially; my test passed just fine. But when I modified B from "printf ; call A" to "printf ; call A ; another printf" it no longer works (the whole thing hangs, which probably points to a server crash), so it seems something is going wrong with relocation after all. Will continue investigating this.

@edsko
Copy link
Collaborator Author

edsko commented Aug 20, 2014

@snoyberg I have tracked this down to a bug in the ghc linker: https://ghc.haskell.org/trac/ghc/ticket/9481. At this point we have 3 options:

  1. Try to fix the bug.
  2. Wait for somebody else to fix the bug.
  3. Try to work around the bug by unloading object files when symbol resolution fails and then load them all again when another object files is added. I had a look at ghci and it seems that that is exactly what it does, although if it does it for this reason or for another, I don't know.

Let me know how you would like me to proceed.

@edsko
Copy link
Collaborator Author

edsko commented Aug 20, 2014

Actually, given that ghci does this anyway, it's probably best to unload object files every time. I will implement that workaround now. We should still keep an eye on that linker bug however.

@snoyberg
Copy link
Member

@edsko I agree with taking that as a first step here.

edsko added a commit that referenced this issue Aug 20, 2014
@edsko
Copy link
Collaborator Author

edsko commented Aug 20, 2014

@snoyberg Ok, I have changed the code so that we unload all objects and reload them all again whenever any C files is added or changed. I feel happier about this anyway, now we don't have to worry about symbols not being re-resolved when addresses change etc. Added some stress tests for this and it all seems fine. Also, the ghc linker bug appears to be OSX specific according to the ticket, so we don't have to worry about that either. I am closing this issue as resolved.

@edsko edsko closed this as completed Aug 20, 2014
@edsko
Copy link
Collaborator Author

edsko commented Sep 5, 2014

@snoyberg FYI, have just pushed a fix where we now capture the output from ghc when object loading fails and report it to the client (not entirely trivial because this causes a hard server crash, not just an exception).

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

No branches or pull requests

3 participants