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

library-exports raises an exception for compiled library that is not yet imported #315

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

owaddell
Copy link
Contributor

@owaddell owaddell commented Jun 4, 2018

Calling library-exports on a compiled library raises an exception if the library has not yet been imported.

If we compile testfile-li3.ss and start a new session, we get:

> (load-library "testfile-li3.so")
> (library-exports '(testfile-li3))
Exception in library-exports: unexpected binding (global . #{testfile-li3 dxvp567u7nmy7h0a66skcx-272})
Type (debug) to enter the debugger.

With the change on this branch, if we recompile testfile-li3.ss and start a new session, we get:

> (load-library "testfile-li3.so")
> (library-exports '(testfile-li3))
(x)

Is this approach misguided? If not, maybe we can tidy up and cherry-pick the next time we have a compelling reason to rebuild the boot files. (Right now it's mingled with the changes for #310.)

The original chi-top-library had installed the interface-binding with different values for token in the call to $sc-put-cte (#f) vs. the call to build-cte-install (*system*). I moved the work of the residual call into import-library and preserved the *system*, but perhaps that should have been #f originally, based on the comment in chi-top-library.

@owaddell
Copy link
Contributor Author

@akeep, does this look like a reasonable way to address the issue outlined above?

The build failure on ta6osx is puzzling. We could not reproduce this on a MacBook Pro running OS X Sierra 10.12.6. The travis build exceeded the maximum time limit for jobs because it seems to have become overly fond of running unix.ms, examples.ms, ieee.ms, date.ms, exceptions.ms, and oop.ms.

@burgerrg
Copy link
Contributor

The master branch ran into a similar timeout on ti3osx, so I restarted the ta6osx tests.

@akeep
Copy link
Contributor

akeep commented Aug 30, 2018

@owaddell this seems like a nice interface improvement, in that it allows a certain amount if inspection without actually needing to execute any of the code in the library.

I have mixed feelings about pulling the interface binding code:

($sc-put-cte uid (libdesc-interface-binding desc) '*system*)

out of the import-code thunk, because it feels like we are in a bit of a strange point in the space when you add up my change to put the visit code bindings in the libdesc-visit-id* so that I could restore them in visit-library failed. It means we have the library interface and visit ids as data in the libdesc record (really the ctdesc record), we have the code for installing the interface in import-library and the code for setting the visit id* in the thunk, but also the visit identifier list in the ctdesc-export-id*. It makes me wonder if we should just get rid of the import-code thunk entirely, and add the invoke identifiers to the ctdesc data structure as well.

The other interesting piece of this is that you and I have made different decisions about where to put this information. I made the visit ids part of the $install-library-ct/code call, where you've added the interface information to the library-ct/info record. It may be it just has to be separated this way, because the binding expressions are potentially not data, but effectively I think they are currently all data for the bindings created in the import-code thunk. It makes me wonder if we should handle all of this information the same way.

Sorry for the somewhat disconnected observations, I'm just wondering if moving to a more data structural representation for all of this and treating the import consistently would have some benefits for us.

@burgerrg
Copy link
Contributor

@owaddell, what's the status of this work?

@owaddell
Copy link
Contributor Author

Here's a different approach. I think this is more self-contained.

What do you think @akeep ?

@@ -2700,6 +2700,12 @@ in fasl files does not generally make sense.
%-----------------------------------------------------------------------------
\section{Bug Fixes}\label{section:bugfixes}

\subsection{\scheme{library-exports} for libary that is not yet imported (9.9.9)}
Copy link
Contributor

Choose a reason for hiding this comment

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

libary => library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear. Thanks for finding this. Somehow my spell checker didn't flag it.

@akeep
Copy link
Contributor

akeep commented Nov 8, 2023

@owaddell, I like this as a minimal approach to enabling this functionality. It is too bad we don't actually need to do anything with the extra argument in the generated case-lambda, but I like that approach better than adding an additional field for this.

@owaddell owaddell merged commit 352f7f5 into cisco:main Nov 8, 2023
13 checks passed
@owaddell
Copy link
Contributor Author

owaddell commented Nov 8, 2023

Thanks for the review, Andy. I agree about the case-lambda, btw.

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.

4 participants