Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug][move-compiler-v2] Spurious Unused assignment warnings with inlining in certain tests #13880

Open
brmataptos opened this issue Jul 1, 2024 · 3 comments
Assignees
Labels

Comments

@brmataptos
Copy link
Contributor

brmataptos commented Jul 1, 2024

🐛 Bug

I've only seen these messages in some big runs, where V2 is used to pre-compile libraries for aptos-transactional-test-harness (for V2 tests).

% UB=1 cargo test --profile ci -p move-compiler -p move-compiler-v2 -p move-compiler-v2-transactional-tests -p move-compiler-transactional-tests -p aptos-transactional-test-harness |& tee test.out2
...

test runner::aptos_test_harness/randomness_safety.move                 ... ok
warning: Unused assignment to `to`. Consider removing or prefixing with an underscore: `_to`
    ┌─ /Users/brm/code/aptos-core/aptos-move/framework/aptos-framework/sources/object.move:544:16
    │
544 │     inline fun transfer_raw_inner(object: address, to: address) acquires ObjectCore {
    │                ^^^^^^^^^^^^^^^^^^
    ·
621 │         transfer_raw_inner(object_addr, BURN_ADDRESS);
    │         --------------------------------------------- from a call inlined at this callsite

warning: Unused assignment to `c`. Consider removing or prefixing with an underscore: `_c`
     ┌─ /Users/brm/code/aptos-core/aptos-move/framework/aptos-stdlib/sources/math64.move:41:23
     │
  41 │     public inline fun mul_div(a: u64, b: u64, c: u64): u64 {
     │                       ^^^^^^^
     │
     ┌─ /Users/brm/code/aptos-core/aptos-move/framework/aptos-framework/sources/delegation_pool.move:2000:13
     │
2000 │             math64::mul_div(active - pool_active, pool.operator_commission_percentage, MAX_FEE)
     │             ----------------------------------------------------------------------------------- from a call inlined at this callsite

warning: Unused assignment to `c`. Consider removing or prefixing with an underscore: `_c`
     ┌─ /Users/brm/code/aptos-core/aptos-move/framework/aptos-stdlib/sources/math64.move:41:23
     │
  41 │     public inline fun mul_div(a: u64, b: u64, c: u64): u64 {
     │                       ^^^^^^^
     │
     ┌─ /Users/brm/code/aptos-core/aptos-move/framework/aptos-framework/sources/delegation_pool.move:2008:13
     │  
2008 │ ╭             math64::mul_div(
2009 │ │                 pending_inactive - pool_pending_inactive,
2010 │ │                 pool.operator_commission_percentage,
2011 │ │                 MAX_FEE
2012 │ │             )
     │ ╰─────────────' from a call inlined at this callsite

The code currently looks like:

module aptos_framework::object {
   ...
    inline fun transfer_raw_inner(object: address, to: address) acquires ObjectCore {
        let object_core = borrow_global_mut<ObjectCore>(object);
        if (object_core.owner != to) {
            if (std::features::module_event_migration_enabled()) {
                event::emit(
                    Transfer {
                        object,
                        from: object_core.owner,
                        to,
                    },
                );
            };
            event::emit_event(
                &mut object_core.transfer_events,
                TransferEvent {
                    object,
                    from: object_core.owner,
                    to,
                },
            );
            object_core.owner = to;
        };
    }
    ...
    public entry fun burn<T: key>(owner: &signer, object: Object<T>) acquires ObjectCore {
        let original_owner = signer::address_of(owner);
        assert!(is_owner(object, original_owner), error::permission_denied(ENOT_OBJECT_OWNER));
        let object_addr = object.inner;
        move_to(&create_signer(object_addr), TombStone { original_owner });
        transfer_raw_inner(object_addr, BURN_ADDRESS);
    }
    ...

@brmataptos brmataptos added the bug Something isn't working label Jul 1, 2024
@brmataptos brmataptos changed the title [Bug][move-compiler] Spurious Unused assignment warnings withinlining [Bug][move-compiler] Spurious Unused assignment warnings with inlining Jul 1, 2024
@brmataptos brmataptos changed the title [Bug][move-compiler] Spurious Unused assignment warnings with inlining [Bug][move-compiler-v2] Spurious Unused assignment warnings with inlining in certain tests Jul 3, 2024
@brmataptos brmataptos assigned brmataptos and fEst1ck and unassigned brmataptos Jul 3, 2024
@brmataptos
Copy link
Contributor Author

brmataptos commented Jul 3, 2024

You can reproduce the test output along with useful information for debugging with

MVC_LOG=trace MVC_BACKTRACE=1 RUST_BACKTRACE=1 UB=1 cargo test --profile ci -p move-compiler -p move-compiler-v2 -p move-compiler-v2-transactional-tests -p move-compiler-transactional-tests -p aptos-transactional-test-harness >& test.out25

That produces a 583MB file, so don't load it in an editor. Load it in less, and search (/) first for Unused assignment, showing something like:

ESC[0mESC[1mESC[38;5;11mwarningESC[0mESC[1m: Unused assignment to `to`. Consider removing or prefixing with an underscore: `_to`
Backtrace: Backtrace [
    { fn: "std::backtrace_rs::backtrace::libunwind::trace", file: "/rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/../../backtrace/src/backtrace/libunwind.rs", line: 105 },
    { fn: "std::backtrace_rs::backtrace::trace_unsynchronized", file: "/rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/../../backtrace/src/backtrace/mod.rs", line: 66 },

Now, search backwards (?) for object\.move:621:

 17: move_to<object::TombStone>($t9, $t11)
     # at /Users/brm/code/aptos-core/aptos-move/framework/aptos-framework/sources/object.move:544:16+18
     # live vars: $t6
 18: $t12 := move($t6)
     # at /Users/brm/code/aptos-core/aptos-move/framework/aptos-framework/sources/object.move:621:41+12
     # live vars: $t12
 19: $t14 := 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
     # at /Users/brm/code/aptos-core/aptos-move/framework/aptos-framework/sources/object.move:544:16+18
     # live vars: $t12, $t14
 20: $t13 := move($t14)
     # at /Users/brm/code/aptos-core/aptos-move/framework/aptos-framework/sources/object.move:545:27+37
     # live vars: $t12

Note that $t12 is assigned and is live at instruction 18. Why is it being flagged as unused?

@brmataptos
Copy link
Contributor Author

I've managed to reproduce the test failure with a slightly smaller test output:

MVC_LOG=trace MVC_BACKTRACE=1 RUST_BACKTRACE=1 UB=1 cargo test --profile ci -p aptos-transactional-test-harness v2-tests/smoke_test >& test.out26

This time it's only 210MB. Still too big for an editor.

brmataptos added a commit that referenced this issue Jul 3, 2024
@brmataptos
Copy link
Contributor Author

I added a simpler test case in third_party/move/move-compiler-v2/tests/unused-assignment/object_test.move
on branch https://github.com/aptos-labs/aptos-core/tree/brm-issue-13880. Enjoy.

brmataptos added a commit that referenced this issue Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Assigned
Development

No branches or pull requests

2 participants