Extend entrypoint generation to handle multi-module packages #374

dmakarov · 2023-09-19T17:34:53Z

Motivation

Current generation of entrypoint glue code works only for packages with entry functions defined in a single module. These changes move entrypoint glue code generation outside ModuleContext. The glue code is generated in a separate LLVM module, so that it can make calls to entry functions defined in any module of a package.

Test Plan

Added rbpf-tests test with entry functions in different modules.

Resolves #369

dmakarov · 2023-09-19T21:13:29Z

language/move-stdlib/tests/fixedpoint32_tests.move

@@ -1,5 +1,5 @@
 #[test_only]
-module std::fixed_point32_tests {
+module std::fp32_tests {
    use std::fixed_point32;


This change of name is to pass the 64 char limit on external symbol length imposed by the Solana VM.

ksolana · 2023-09-20T02:01:31Z

language/solana/move-to-solana/src/lib.rs

-    // NB: context must outlive llvm module
-    // fixme this should be handled with lifetimes
+    // FIXME: this should be handled with lifetimes.
+    // Context (global_cx) must outlive llvm module (entry_llmod).


@brson's expertise is needed here.

I'll give it a look.

I filed an issue with a potential fix for this #379

I don't think it should hold up this pr, as this pr is just continuing the existing pattern of manually managing the lifetime of the llvm mod.

fwiw I also recall thinking that the original scheme for managing lifetimes of these contexts, with the 'mm and 'up lifetimes, is probably wrong, though I don't remember why. Anyway, that scheme appears to still be working for now.

ksolana · 2023-09-20T02:05:24Z

language/solana/move-to-solana/src/stackless/entrypoint.rs

+ * requested entry function to be passed in instruction_data byte
+ * array. The logic in solana entrypoint iteratively compares the
+ * string slice passed in instruction_data to every entry function
+ * symbol of the module. Once a matching entry function is found,


Is there a chance that the list of entry functions can have duplicates?

or ODR takes care of it?

if two modules have the same name and they both contain entry functions with the same name, then yes there will be duplicates. We probably will include modules’ unique addresses in function names to avoid conflicts.

maybe it will help to assert that there are no two duplicates. at least in debug mode?

I added a debug_assert!() to check for duplicate entry function names.

ksolana · 2023-09-20T05:24:05Z

language/tools/move-mv-llvm-compiler/src/main.rs

+                let level = record.level();
+                let mut style = formatter.style();
+                match record.level() {
+                    Level::Error => style.set_color(Color::Red),


nice! how do we test the logger?

We could probably write tests checking the log messages. Is it worth the effort at the moment? I added this initializer for consistency with move-cli code path, as previously the logger was initialized when global context was created which is too late and wrong (https://github.com/jcivlin/move/blame/599dbfbecc6d8b8893ee9c683b402e740f1f9820/language/tools/move-mv-llvm-compiler/src/stackless/translate.rs#L121-L142). I mentioned previously, I'd like to decommission move-mv-llvm-compiler as it will never be used by the end users, it is only used for testing at the moment, and it is taking wasted efforts to maintain.

not necessary to write test for it atm. i'll just create an issue for this and move on. #377

dmakarov · 2023-09-20T14:16:23Z

language/solana/move-to-solana/src/stackless/module_context.rs

+        llvm_module: &'up llvm::Module,
+        rtty_cx: &RttyContext,
+        val: u64,
+    ) {


This and several other methods need to be used by EntrypointGenerator which has its own LLVM module that isn't included in an existing ModuleContext, therefore I tried to reuse the code by converting these methods to 'class'-level methods instead of 'instance'-level. Maybe some other refactoring is needed to have a better design and code reuse. I would appreciate suggestions how to improve this.

as long as it is helping you make progress, i guess this is fine. Maybe we can just write a comment here (as TODO?) or have a task and move on.

Added a comment.

ksolana · 2023-09-20T17:50:15Z

language/tools/move-mv-llvm-compiler/src/main.rs

+            test_signers: args.test_signers.clone(),
+            ..MoveToSolanaOptions::default()
+        };
+        let entry_llmod = global_cx.llvm_cx.create_module("solana_entrypoint");


nit: i feel like we need a file with all these constants (e.g. solana_entrypoint) which can be reused. As in language/tools/move-mv-llvm-compiler/tests/rbpf-tests.rs where we have solana_entrypoint.o

I agree, although rbpf-tests.rs is exposed to too many internals of compiler at the moment. This specific string is supposed to be hidden in move-to-solana/lib.rs only, where it should be defined as a compile time constant, probably.

brson

I'm fine with all this assuming @ksolana's is satisfied.

dmakarov force-pushed the dm/altentry branch 5 times, most recently from 49797b2 to 40683a7 Compare September 19, 2023 21:05

dmakarov commented Sep 19, 2023

View reviewed changes

dmakarov force-pushed the dm/altentry branch from 40683a7 to 6dd52a5 Compare September 19, 2023 22:51

dmakarov marked this pull request as ready for review September 19, 2023 23:11

dmakarov requested review from nvjle, brson and ksolana September 19, 2023 23:11

dmakarov force-pushed the dm/altentry branch from 6dd52a5 to 4e97d2d Compare September 19, 2023 23:21

ksolana reviewed Sep 20, 2023

View reviewed changes

dmakarov force-pushed the dm/altentry branch 2 times, most recently from 9c35610 to 56f7cde Compare September 20, 2023 12:10

dmakarov commented Sep 20, 2023

View reviewed changes

ksolana reviewed Sep 20, 2023

View reviewed changes

Extend entrypoint generation to handle multi-module packages

15116df

dmakarov force-pushed the dm/altentry branch from 56f7cde to 15116df Compare September 20, 2023 18:05

brson approved these changes Sep 20, 2023

View reviewed changes

ksolana approved these changes Sep 21, 2023

View reviewed changes

dmakarov merged commit 6716e0e into anza-xyz:llvm-sys Sep 21, 2023
8 checks passed

dmakarov deleted the dm/altentry branch September 21, 2023 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend entrypoint generation to handle multi-module packages #374

Extend entrypoint generation to handle multi-module packages #374

dmakarov commented Sep 19, 2023 •

edited

dmakarov Sep 19, 2023

ksolana Sep 20, 2023

brson Sep 20, 2023

brson Sep 20, 2023 •

edited

brson Sep 20, 2023 •

edited

ksolana Sep 20, 2023

ksolana Sep 20, 2023

dmakarov Sep 20, 2023

ksolana Sep 20, 2023

dmakarov Sep 20, 2023

ksolana Sep 20, 2023

dmakarov Sep 20, 2023

ksolana Sep 20, 2023

dmakarov Sep 20, 2023

ksolana Sep 20, 2023

dmakarov Sep 20, 2023

ksolana Sep 20, 2023

dmakarov Sep 20, 2023

brson left a comment

Extend entrypoint generation to handle multi-module packages #374

Extend entrypoint generation to handle multi-module packages #374

Conversation

dmakarov commented Sep 19, 2023 • edited

Motivation

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson Sep 20, 2023 • edited

Choose a reason for hiding this comment

brson Sep 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson left a comment

Choose a reason for hiding this comment

dmakarov commented Sep 19, 2023 •

edited

brson Sep 20, 2023 •

edited

brson Sep 20, 2023 •

edited