Add Template: Native Simulator Debug #14

joii2020 · 2024-07-12T14:49:46Z

Using the ckb-debugger for contract development can be quite cumbersome. Therefore, we offer an alternative debugging method:

Compile the contract into a native platform dynamic library and use the ckb-testtool to run this dynamic library similarly to a contract. This allows developers to debug the contract directly using their existing IDE.

Creation

When creating a contract, a native dynamic library named <contract_name>-dbg will be created in the contract directory. If you do not want to create this project, use the parameter ALSO_ADD_DEBUGGER=false.

Usage

Run make build-simulator to complete the compilation. Once compiled, during debugging, open the tests directory in the project and enable the features: simulator. This will automatically load and execute the native simulator.

xxuejie · 2024-08-12T05:38:42Z

Personally, I think the change here is far too intrusive here:

First of all, there is absolutely no documents on how the feature works, one has to guess from the changed code
The simulator part requires its own, separate crate to work, there is an option ALSO_ADD_DEBUGGER(we haven't even talked about why this option is left to on by default), one can turn it to off to skip the debugger code generation. However, the contract template is modified so build-simulator is always created, assuming someone does not generate the debugger code when generating a template, this build-simulator command would not make sense at all. I would recommend spend time thinking about how we can ensure the workflow is good enough for cases where simulator code is present, and the simulator code is missing.
The choice of folders to put the simulator code is really, messy, in my humble opinion. We are putting the generator code to contracts/i-am-a-contract/i-am-a-contract-dbg here. First of all, what does dbg mean here? Is a native simulator the only way to debug a contract? Or are you intending on enhancing here so it covers more different types of debugging solutions? Second, the folder structure could really use some changes. It makes more sense to me to use a structure like native-simulators/i-am-a-contract. What's even better here, is that when the structure like native-simulators/i-am-a-contract is used, you can just use make build in this folder, knowing that you are building a simulator, you won't need this weird commands like make build-simulator anymore. And it also solves the previous problem: when native-simulators/i-am-a-contract is missing, we know there is a contract that does not have accompanying native simulator code.
The template name add-debugger could use some changes, we had some real unfortunate naming in the past, I would recommend we pay more attentions from now on.
Similar to chore: generate unit test code #15 (comment), this repository actually contains a handful of different contract templates other than contract, if changes to contract template are necessary, please also make changes to other templates.

This is all I have for now, tho I do want to take another look again when the above issues are resolved.

xxuejie · 2024-08-12T05:43:31Z

There are actually 3 different names used, so as to refer to the same thing:

debugger in add-debugger folder name
simulator in PR description, as well as feature name
dbg used in generated crate name

Please make sure to use the same word to refer to the same thing, across all parts of code changes.

xxuejie · 2024-08-12T05:54:58Z

One additional point: I wasn't sure if there is the need for scripts/add-debugger file. We should be able to simplify the workflow, so all we need is a few lines in the makefile.

xxuejie

I noticed the documentation is removed, care to explain why?

xxuejie · 2024-09-03T00:34:44Z

native-simulator/README.md

@@ -0,0 +1,7 @@
+# {{project-name}}
+
+TODO: Write this readme


Please state it clearly that this template is used to provide native simulator for a particular contract, and is not designed to be used on its own.

Right now this README resembles the README for a typical contract too much.

xxuejie · 2024-09-03T00:37:07Z

native-simulator/cargo-generate.toml

+
+[placeholders]
+contract_name = { prompt = "contract name", type = "string" }
+contract_crate_name = { prompt = "contract crate name", type = "string" }


2 things:

Right now the contracts generated will use the same value as contract_name and contract_crate_name, so there is no point in requesting 2 values here.

The current convention, is to provide values as makefile arguments, such as make CONTRACT=iamacontract. I'm not saying we cannot use prompts, but if you do need prompts over makefile arguments here, please spend the time to elaborate the reason.

xxuejie · 2024-09-03T00:37:30Z

stack-reorder-contract/src/main.rs

@@ -12,15 +12,15 @@ ckb_std::entry!(program_entry);
 default_alloc!();

 pub fn program_entry() -> i8 {
-    let mut x: u64;
+    let mut _x: u64;


Can you share the reason leading to those changes?

In some cases, there will be a warning here.
https://github.com/cryptape/ckb-script-templates/actions/runs/10552002309/job/29230208410#step:11:474

I would recommend diving more into those cases. What really happened there? Why the assembly line is not processed at all?

Even for some reasons, we cannot work around the problems, to me it's better to explicitly add statements like #[allow(unused_variables)] and #[allow(unused_assignments)]. To me it might not be a good thing to access a variable with _ prefix.

xxuejie · 2024-09-03T00:38:31Z

workspace/Cargo.toml

+
+# TODO: Will be deleted after release
+[replace]
+"ckb-std:0.15.3" = { git = "https://github.com/nervosnetwork/ckb-std.git", branch = "master" }


Please lock specific version via rev, right now this is far too vague.

Normally, this code shouldn't be here. However, since the modified ckb-std hasn't been released yet, I've used replace.

I do understand the rationale behind those lines(waiting on a new version of ckb-std), but I believe they are separate issues: if you want to propose such a change in the PR here, make sure it is added properly using rev, otherwise you should leave it out.

xxuejie · 2024-09-03T00:40:24Z

workspace/scripts/generate-native-simulator

@@ -0,0 +1,75 @@
+#!/usr/bin/env bash
+#
+# Add code for native debugging to existing onchain-script


I still don't see why a full script is required here. To me we should aim to make the whole process as simple as a single cargo generate, it would be helpful if you can share the reason why you need a script here.

A native simulator project must correspond to a specific contract. Therefore, users should be guided to select an existing contract, which will also help standardize the naming of native simulator projects.

After the project is generated, some modifications need to be made to the contract for it to work. For example, the target contract should have features: simulator added, along with some changes in main.rs related to the simulator.

Regarding point 1, generating both a contract and a cdylib in the same project can lead to some unavoidable warnings and increase code complexity.
As for point 2, while developers can handle the necessary code modifications themselves, I believe it would be more convenient to include them automatically.

Let's dissect the included script bit by bit as of this commit version:

Line 6 - line 49 are just trying to select the contract to generate simulator code. For a standalone script, it might be helpful but here we are integrating the workflow in the makefile. Note that a makefile actually executes the script, not an individual user. This way, I don't see the reason providing those flexibilities. The makefile shall already know the exact contract name to generate simulator for.

Line 54 actually does the bulk of the work but this is a single invocation of cargo generate, we can simply include it in the workspace-level makefile

Line 55 could really have the potential of becoming a mysterious act that confuses a lot of people. And what it really does to me, is just to tweak the behavior slightly, in the presense of simulator feature. We can just modify existing contract templates with a new lib.rs file, to me it won't cause problem either way(the newly added code is all under a simulator feature).

Line 57 - 61 confuses me: why does native-simulators directory needs its own makefile? Why not change the workspace level makefile? See Compile native crates in make build as well #16 for an example for customizing crates in certain folders

Line 64 - 69, IMO, can also be incorporated in current contract templates. They won't affect the compilation process when native simulator is missing

Line 71 - 75 are also lines that can be included in current templates. They are just feature depended configs.

I don't see any part in this script, that has to do with cdylib.

One more thought: I really recommend that we shall rename the feature here to native-simulator. The single word simulator is far too vague to me. But changing it here alone won't help(actually to me it does more harm if we name the feature native-simulator here but simulator in ckb-std), if we want to make the change, we should only make the change when we can change the same feature name in ckb-std.

Can I generate a native-simulator directory in the workspace? There will be Cargo.toml and Makefile in it, so that I don't need to deal with this part every time I create a new native-simulator project?

Is there really the need to have separate Cargo.toml and Makefile in the root folder of native-simulator directory? What problem do they solve?

Why not use Cargo.toml file at the root of workspace folder?

And this is really my biggest concern here: we already have Cargo.toml and Makefile at the root of the workspace folder, why bother adding a new set of those 2 files?

Is it appropriate to put this kind of project specifically used for debugging in the project root directory?

They are Rust crates, I don't see why now(unless of course, some feature conflict will require you to use a different Rust-cargo-workspace-level Cargo.toml, that will be a different story). For example, the Cargo.toml at workspace root directory contains a tests crate for writing tests, I see no difference between tests crate and simulator crates.

So my suggestion here is: if we can use a single Cargo.toml file, we should definitely stick to one, but if there is other requirements(I suspect there is a feature conflict: contracts have to be built without simulator feature, while simulator crates require simulator feature), a proper way can be:

We add a new native-simulator-workspace template, containing Cargo.toml and Makefile for simulators.

When generating a new simulator, first test if native-simulator directory exists, if not, use native-simulator-workspace template to populate this directory first

Generate the simulator crate for the contract specified in native-simulator directory then.

OK, I will modify it.

joii2020 · 2024-09-03T11:47:28Z

I noticed the documentation is removed, care to explain why?

This document will be moved to the ckb-standalone-debugger after revisions. I believe users who need to debug will naturally use ckb-debugger, so it makes sense to place the documentation here.

xxuejie · 2024-09-04T01:01:05Z

I noticed the documentation is removed, care to explain why?

This document will be moved to the ckb-standalone-debugger after revisions. I believe users who need to debug will naturally use ckb-debugger, so it makes sense to place the documentation here.

It is fine if the majority of the documentation shall be put in ckb-debugger repository, but here a big feature is introduced, it is necessary at least to add certain notes, such as how a native simulator crate can be generated and all its used arguments. Then you can add a link to the documentation at ckb-debugger side introducing the debugging process.

joii2020 · 2024-09-04T01:47:52Z

I noticed the documentation is removed, care to explain why?

This document will be moved to the ckb-standalone-debugger after revisions. I believe users who need to debug will naturally use ckb-debugger, so it makes sense to place the documentation here.

It is fine if the majority of the documentation shall be put in ckb-debugger repository, but here a big feature is introduced, it is necessary at least to add certain notes, such as how a native simulator crate can be generated and all its used arguments. Then you can add a link to the documentation at ckb-debugger side introducing the debugging process.

Ok, I'll add it later

xxuejie · 2024-09-05T03:59:43Z

workspace/Cargo.toml

@@ -12,3 +12,12 @@ members = [
 overflow-checks = true
 strip = true
 codegen-units = 1
+
+[profile.ckb-debug]


What does this profile do and where is it used?

When using ckb-debugger to debug, a contract with a symbol file is required, so this profile is added.
I'll add a comment later.

If symbols are the only blocker, we can ask a bigger question: is it true that the workspace level cargo file strips all symbols by default? What if we change the above strip from true to false, then in makefile, use a separate step to strip the binary, so actually here we are providing both types of binary? To me that's a better solution than adding a new profile.

This is better, but developers need to install llvm additionally. (llvm-objcopy)

Honestly I'm fine with requiring LLVM to be present all the time.

xxuejie · 2024-09-05T04:00:13Z

workspace/Makefile

@@ -48,6 +48,20 @@ build:
 		$(MAKE) -e -C contracts/$(CONTRACT) build; \
 	fi

+build-simulator:


Why not build the simulators when executing make build? Why use a separate command?

I think that most of the time, developers don’t need native-simulator, so I divide it into two commands here.
Or for convenience, add a dependency build to build-simulator ?

I would personally stick to a single make build command, it avoids potential build inconsistencies(what if someone types make test but do not type make build-simulator). If you don't need native-simulator, don't generate the crate.

When there is no build-simulator, the contract will be executed. And tests need to add the features: simulator.

We are talking about different points. Assuming there is make build and make build-simulator, what if someone executes one but not the other? Are all intermediate files compiled from the same source code? What if there are the differences?

To me having a single make build is much better when it takes care of everything, ther e is less confusion.

In addition, when one opts to generate the simulator code for one contract, he / she should already know they will pay the extra cost, including compilation time.

After build-simulator is merged with build, native-simulator files will be generated in build/release. Is this okay?

I don't have a problem with it but I do want to ask why this is the case? The files in build/release are manually copied over, what's the problem against tweaking this?

In this way, ckb-testtool can automatically load these libs.

I must say I don't really understand the reasons here. Seems like just work, not real blockers.

xxuejie · 2024-09-05T04:00:53Z

workspace/Makefile

+		--destination native-simulators \
+		-d contract_name=$(CRATE) \
+		-d contract_crate_name=`echo "$(CRATE)" | tr '-' '_'`; \
+	mv native-simulators/$(CRATE)-sim/src/contract-lib.rs contracts/$(CRATE)/src/lib.rs; \


This simply shifts the complexity discussed here from a script, to make command. It does not truly simply the workflow, please fix this.

If create a native-simulator, we will have to modify the corresponding contract. Or should these modifications be added directly to the contract template, or instructions be provided so that developers can modify them themselves?

Based on the content of the changes, I would favor directly adding those changes to current templates(notice there are quite a few templates other than contract), if you want, you can write some instructions in the README part on how to migrate existing contracts over with simulator support.

xxuejie · 2024-09-05T04:01:19Z

workspace/tests/Cargo.toml

@@ -3,6 +3,10 @@ name = "tests"
 version = "0.1.0"
 edition = "2021"

+[features]
+# default = [ "simulator" ]


What is this line for?

xxuejie · 2024-09-10T08:32:05Z

workspace/Makefile

+	cargo generate $(TEMPLATE_TYPE) $(TEMPLATE_REPO) native-simulator \
+		-n $(CRATE)-sim \
+		--destination native-simulators \
+		-d contract_name=$(CRATE) \


I think there is a comment unresolved that got lost in history. Personally, I don't see the need to add prompts here. Cargo generate does provide enough filters in templates:

https://cargo-generate.github.io/cargo-generate/templates/index.html#additional-liquid-filters

https://shopify.github.io/liquid/filters/remove/

I believe we can derive the values of contract_name and contract_crate_name from the builtin placeholders project-name and crate_name

xxuejie · 2024-09-10T08:42:00Z

workspace/Makefile

-	fi
+	fi; \
+	for sim in $(wildcard native-simulators/*); do \
+		cd $$sim && cargo build && cd ../..; \


Please respect MODE_ARGS and CARGO_ARGS here

Does cargo build -p work? If so, we can remove the cd part

Maybe it's worthwhile to test against CONTRACT variable, if it is set, we should build only the simulator for this specific contract.

xxuejie · 2024-09-14T00:57:57Z

native-simulator/Cargo.toml

@@ -5,7 +5,7 @@ edition = "2021"

 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 [dependencies]
-{{contract_name}} = { path = "../../contracts/{{contract_name}}", features = ["native-simulator"] }
+{{project-name | remove: "-sim"}} = { path = "../../contracts/{{project-name | remove: "-sim"}}", features = ["native-simulator"] }


Sorry just want to be picky one more more item(and I believe we should be good here), right now, this solution requires that -sim cannot be used as project name, for example, I cannot name my contract as my-simple-project, since remove filter will remove all occurrences of -sim.

I suggest we apply this pattern, what we can do here is:

project-name | append: "@@SIMULATOR_PLACEHOLDER@@" | remove "-sim@@SIMULATOR_PLACEHOLDER@@"

This can ensure that only the last -sim part is removed, no occurrences of -sim in the middle of the name will be affected.

* build-simulator dependencies build * clean can clear *-dbg

* Contracts support native-simulator by default. * native-simulator project no longer requires scripts to add code to contracts. * simulator renamed to native-simulator.

xxuejie

Just one minor issue, rest is good to me

xxuejie · 2024-09-24T06:58:04Z

native-simulator/src/lib.rs

@@ -0,0 +1 @@
+ckb_std::entry_simulator!({{project-name | remove: "-sim" | replace: "-", "_"}}::program_entry);


Please use the same technique above to append a placeholder first, then remove -sim together with the placeholder.

Sorry, I neglected it, I will change it.

xxuejie

LGTM

2 more points here:

There seems to be a CI error on aarch64 system, might want to fix the relavant code in ckb-std first
For now, cargo file is modified to use a patched ckb-std version. Considering the fact that upgrading dependencies can be a reluctant task, one idea might be to release a new ckb-std version first, then update the code to use a proper ckb-std version. However that will depend on release schedule of ckb-std itself, I don't have strong opinions either way, and will just leave it to you.

joii2020 · 2024-09-24T07:15:10Z

LGTM

2 more points here:

There seems to be a CI error on aarch64 system, might want to fix the relavant code in ckb-std first

For now, cargo file is modified to use a patched ckb-std version. Considering the fact that upgrading dependencies can be a reluctant task, one idea might be to release a new ckb-std version first, then update the code to use a proper ckb-std version. However that will depend on release schedule of ckb-std itself, I don't have strong opinions either way, and will just leave it to you.

I'm dealing with this problem on ckb-std.

xxuejie · 2024-11-01T07:53:32Z

contract/Makefile

@@ -6,7 +6,7 @@ TOP := $(cur_dir)
 # RUSTFLAGS that are likely to be tweaked by developers. For example,
 # while we enable debug logs by default here, some might want to strip them
 # for minimal code size / consumed cycles.
-CUSTOM_RUSTFLAGS := --cfg debug_assertions
+CUSTOM_RUSTFLAGS := -C debug_assertions


Are those changes still needed?

There will be an error here.

They should have made the changes here

joii2020 force-pushed the main branch 2 times, most recently from 2cdcb71 to 15645dc Compare August 1, 2024 05:35

joii2020 changed the title ~~Support add debugger~~ Add Template: Native Simulator Debug Aug 2, 2024

xxuejie reviewed Sep 3, 2024

View reviewed changes

joii2020 force-pushed the main branch from 6d82ba0 to d4a1b97 Compare September 4, 2024 09:25

xxuejie reviewed Sep 5, 2024

View reviewed changes

joii2020 force-pushed the main branch 2 times, most recently from cb1b983 to 138548f Compare September 10, 2024 08:22

xxuejie reviewed Sep 10, 2024

View reviewed changes

xxuejie reviewed Sep 14, 2024

View reviewed changes

joii2020 added 13 commits September 24, 2024 14:26

Support add debugger

23f8e89

remove default_alloc

1346f63

Auto add feature simulator to contract

1304d24

Optimize the process of adding simulator

dce5def

Add load_binary_dbg and support native simulator debug

c77a4f3

Fix error

6bbadcd

revert tests/lib.rs

7a7e7cc

test add feature simulator

b5697f6

Optimizing Makefile dependencies

c946ef2

* build-simulator dependencies build * clean can clear *-dbg

Fix some bugs in add-debugger

756ddb2

ckb-testtool version update to 0.12.0

d45d74a

Some dependencies are replaced with those in git

41aa9e3

Update ckb-testtool version

e903a59

joii2020 added 11 commits September 24, 2024 14:26

Fix make check error when add native simulator

6517a72

Add a comment about profile.ckb-debug

aa8a2f0

Delete the commented out code

b98b2a3

build-simulator merge to build

8403fce

The built contract has symbols and is deleted using objcopy

d30d2e3

ckb-std update to 0.15.3

30f44d6

Contract add feature: native-simulator

39da951

* Contracts support native-simulator by default. * native-simulator project no longer requires scripts to add code to contracts. * simulator renamed to native-simulator.

Fix error: x64-simulator-crate simulator rename to native-simulator

e19c9b3

Optiomization workspace/Makefile build

09fdcde

Using Liquid to process templates

1ff8ac9

Optimize cargo-generate rules

d81d8d0

joii2020 force-pushed the main branch from 24e1f3d to d81d8d0 Compare September 24, 2024 06:37

xxuejie reviewed Sep 24, 2024

View reviewed changes

Optimize cargo-generate rules in native-simulator lib.rs

a0a0d4e

xxuejie approved these changes Sep 24, 2024

View reviewed changes

Update ckb-std rev

3215d67

joii2020 force-pushed the main branch from afcf5ea to 3215d67 Compare September 24, 2024 07:56

joii2020 added 3 commits November 1, 2024 11:22

Update dependencies: ckb-std and ckb-testtool

2ea0734

Fix CI

ab116fa

Fix CI: on Makfile about CUSTOM_RUSTFLAGS

8e8a9d9

joii2020 force-pushed the main branch 2 times, most recently from ea81591 to fa78d67 Compare November 1, 2024 07:50

xxuejie reviewed Nov 1, 2024

View reviewed changes

Update ckb-std to 0.16.3

9db520e

joii2020 force-pushed the main branch from fa78d67 to 9db520e Compare November 1, 2024 09:21

Typos: Makefile CUSTOM_RUSTFLAGS

b6d79b9

xxuejie approved these changes Nov 12, 2024

View reviewed changes

joii2020 merged commit a9d8dd7 into cryptape:main Nov 12, 2024
8 checks passed

		@@ -0,0 +1 @@
		ckb_std::entry_simulator!({{project-name \| remove: "-sim" \| replace: "-", "_"}}::program_entry);

Add Template: Native Simulator Debug #14

Add Template: Native Simulator Debug #14

Conversation

joii2020 commented Jul 12, 2024 • edited Loading

Creation

Usage

xxuejie commented Aug 12, 2024

xxuejie commented Aug 12, 2024

xxuejie commented Aug 12, 2024

xxuejie left a 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

xxuejie Sep 4, 2024 • edited Loading

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

xxuejie Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joii2020 commented Sep 3, 2024

xxuejie commented Sep 4, 2024

joii2020 commented Sep 4, 2024

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

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

xxuejie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxuejie left a comment

Choose a reason for hiding this comment

joii2020 commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joii2020 commented Jul 12, 2024 •

edited

Loading

xxuejie Sep 4, 2024 •

edited

Loading

xxuejie Sep 4, 2024 •

edited

Loading