ChezScheme icon indicating copy to clipboard operation
ChezScheme copied to clipboard

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

Open owaddell opened this issue 6 years ago • 3 comments

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 avatar Jun 04 '18 03:06 owaddell

@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.

owaddell avatar Aug 27 '18 14:08 owaddell

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

burgerrg avatar Aug 27 '18 15:08 burgerrg

@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.

akeep avatar Aug 30 '18 02:08 akeep

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

burgerrg avatar Oct 18 '23 15:10 burgerrg

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

What do you think @akeep ?

owaddell avatar Oct 20 '23 16:10 owaddell

@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.

akeep avatar Nov 08 '23 04:11 akeep

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

owaddell avatar Nov 08 '23 18:11 owaddell