Add --keep-symbols flag

[athei]

In order to properly analyze a contract binary we need to keep the wasm name section alive so that we can see what is going on. Using the non post processed wasm file is of no use because we are interested in the fully optimized binary when doing an analysis. To that end I added a new --keep-symbols flag to the build command which prevents wasm-opt from removing the debug section.

I needed to make some other changes to make this work, too. Most notable I removed the call to pwasm_utils::optimize which unconditionally removes all custom sections. We don't need this anyways because wasm-opt does the same optimizations and more. It sufficed to remove all exports from the module (but call and deploy) to have the same result without pwasm_utils::optimize. I argue that having one less pass involved is a good thing.

[Robbepop]

How about not adding a new flag (that users have to know) but instead do all this whenever we do a normal --debug build which is the default build config?

[athei]

There is no --debug flag and hence it cannot be the default. The default for build is to build for production. check is for development.

[cmichi]

There is no --debug flag and hence it cannot be the default. The default for build is to build for production. check is for development.

I think what Robin means that in a recent PR we decided against a cargo contract build --debug flag and instead now mirror cargo behavior of building for debug by default. We instead added a cargo contract build --release flag which leaves debug info out.

By debug info we currently only have the ink-debug flag, which enables printing debug statements and hence some overhead needs to be compiled into the contract to support that.

I think the misunderstanding here is that you are probably referring to the fact that we always build contracts as cargo build --release and also still run wasm-opt on them. This is required so that contracts can even be deployed.

So the distinction I think Robin wants to make is that we have a debug and a release mode for cargo-contract, whereas you are talking about cargo build for the *.rs files. So you could just leave the symbols in by default and only omit them on cargo contract build --release.

[athei]

But nothing of those things is a reality on master, right? Because I don't see a --release flag anywhere.

[cmichi]

But nothing of those things is a reality on master, right? Because I don't see a --release flag anywhere.

#298

It's approved, but not yet merged because we need rc4 to be released first.

[athei]

Thanks. I checked the PR. I disagree. --keep-symbols is orthogonal to --release. I want to analyze the optimized binary as it would be deployed on chain (that might or might not include a build without the ink! debug features). So basically I want --keep-symbols --release.

[Robbepop]

If this is the case then please rename the flag to --keep-debug-symbols so that it is clear by name that this is for debug inspection.

[atenjin]

we have create a pr in post #131
but we close it for no response ....
maybe --debug is a good choose for many cases, not only in this situation. we also need those information which contains debug symble in name section

[ascjones]

LGTM

[cmichi]

Thanks! cargo fmt fails right now, but besides that it LGTM.

[athei]

Thanks! cargo fmt fails right now, but besides that it LGTM.

Yeah but not because of the code. Just some intermittent CI failures. Saw this multiple times here:

Error during execution of `cargo metadata`: error: failed to download `env_logger v0.9.0`

[athei]

Merging this. The CI is broken (some caching issue, again). I ran cargo fmt locally.