Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Reorganize move-native #223

Closed
brson opened this issue Jul 8, 2023 · 4 comments · Fixed by #241
Closed

Reorganize move-native #223

brson opened this issue Jul 8, 2023 · 4 comments · Fixed by #241
Labels
bug Something isn't working

Comments

@brson
Copy link
Collaborator

brson commented Jul 8, 2023

It is getting pretty unwieldy and I'm starting to get frustrated finding the routines I'm looking for. Some things to change:

  • move every module to its own file
  • extract all the vector code to its own module, called by both rt and std
  • don't call from the rt module into std - instead have both of them call into a lower-level module
@brson brson added the bug Something isn't working label Jul 8, 2023
@brson
Copy link
Collaborator Author

brson commented Jul 8, 2023

Remove #![allow(unused)].

@nvjle
Copy link

nvjle commented Jul 8, 2023

Hopefully this will address the code around // fixme this is pretty awkward - this is intended to be a no-std crate. We're either going to be no_std or not. Also, I think the panic handler needs to be factored out so that move-native can be linked into different scenarios without colliding with std. Otherwise the handler may be doubly defined at link time (I'm guessing this is the unresolved issue you alluded to in the original comment of #128). This is briefly discussed in https://doc.rust-lang.org/nomicon/panic-handler.html.

@brson
Copy link
Collaborator Author

brson commented Jul 11, 2023

Hopefully this will address the code around // fixme this is pretty awkward - this is intended to be a no-std crate. We're either going to be no_std or not

This comment is a bit misleading as std is only linked in when running move-native's own tests. This is not an uncommon arrangement to be no_std in production, but link to std in tests. Still I am thinking of deleting the drop bomb code as it is pretty confusing. It was helpful during initial testing.

Also, I think the panic handler needs to be factored out so that move-native can be linked into different scenarios without colliding with std. Otherwise the handler may be doubly defined at link time (I'm guessing this is the unresolved issue you alluded to in the original comment of #128)

I believe there were other linkage problems. Dealing with the panic handler is something I would have known how to do.

In what other scenarios does move-native need to link to std? At the moment the panic handler is only configured when targeting solana. If we decide the solana target should use std and/or solana_program we will need to delete or rework the panic handler and global allocator.

@nvjle
Copy link

nvjle commented Jul 12, 2023

Hopefully this will address the code around // fixme this is pretty awkward - this is intended to be a no-std crate. We're either going to be no_std or not

This comment is a bit misleading as std is only linked in when running move-native's own tests. This is not an uncommon arrangement to be no_std in production, but link to std in tests.

Fair enough-- but it does seem awkward-- even if "everyone is doing it", and even if it is conditional.

Still I am thinking of deleting the drop bomb code as it is pretty confusing. It was helpful during initial testing.

Seems reasonable.

One similar guard-rail feature that I would still find useful today (would have saved me twice) is storing "magic numbers" in each of the descriptors (MoveType, MoveUntypedVector, and so forth). This way, if the compiler happens to (say) pass a bad descriptor to the runtime, it would likely be discovered easily (and earlier) instead of mostly processing things and returning bad results that are difficult to track down. Since we'll be processing almost solely financial applications, it does make me a bit nervous.

Also, I think the panic handler needs to be factored out so that move-native can be linked into different scenarios without colliding with std. Otherwise the handler may be doubly defined at link time (I'm guessing this is the unresolved issue you alluded to in the original comment of #128)

I believe there were other linkage problems. Dealing with the panic handler is something I would have known how to do.

In what other scenarios does move-native need to link to std? At the moment the panic handler is only configured when targeting solana. If we decide the solana target should use std and/or solana_program we will need to delete or rework the panic handler and global allocator.

That was the scenario I was thinking of. I'm sure there are others I could dream up, but like you say, let's burn the bridge when we get there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants