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

test: Fix function does not return a value warnings #14708

Merged
merged 2 commits into from
May 31, 2021

Conversation

Patater
Copy link
Contributor

@Patater Patater commented May 26, 2021

Summary of changes

Many test stub functions are meant to return a value, but weren't. Clang would generate a warning for each instance where we weren't returning anything in a function that was meant to return a value.

        warning: non-void function does not return a value [-Wreturn-type]

For a specific example, my_radio::time_on_air() is supposed to return a uint32_t, but wasn't returning anything. We'll return a zero instead of relying on undefined behavior.

Without this, clang 11.0.1 was generating a virtual function implementation with a ud2 instruction to abort at run-time, causing some execution of some unit tests to abort.

        Running main() from gmock_main.cc
        [==========] Running 10 tests from 1 test suite.
        [----------] Global test environment set-up.
        [----------] 10 tests from Test_LoRaPHYUS915
        [ RUN      ] Test_LoRaPHYUS915.constructor
        [       OK ] Test_LoRaPHYUS915.constructor (0 ms)
        [ RUN      ] Test_LoRaPHYUS915.restore_default_channels
        [       OK ] Test_LoRaPHYUS915.restore_default_channels (0 ms)
        [ RUN      ] Test_LoRaPHYUS915.rx_config
        [       OK ] Test_LoRaPHYUS915.rx_config (0 ms)
        [ RUN      ] Test_LoRaPHYUS915.tx_config
        Process 35669 stopped
        * thread #1, name = 'lorawan-loraphy-', stop reason = signal SIGILL: privileged instruction
            frame #0: 0x0000000000276f73 lorawan-loraphy-us915-unittest`my_radio::time_on_air(this=0x0000000800c2b048, modem=MODEM_LORA, pkt_len='\0') at Test_LoRaPHYUS915.cpp:90:5
           87       };
           88
           89       virtual uint32_t time_on_air(radio_modems_t modem, uint8_t pkt_len)
        -> 90       {
           91       };
           92
           93       virtual bool perform_carrier_sense(radio_modems_t modem,
        (lldb) disassemble --pc
        lorawan-loraphy-us915-unittest`my_radio::time_on_air:
        ->  0x276f73 <+67>: ud2
            0x276f75:       int3
            0x276f76:       int3
            0x276f77:       int3
        (lldb)

Impact of changes

None

Migration actions required

None

Documentation

None


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label May 26, 2021
@ciarmcom
Copy link
Member

@Patater, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@LDong-Arm LDong-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please fix indentation in the PR description (e.g. copy the commit message from the editor during amend, instead of the git log).

Many test stub functions are meant to return a value, but weren't. Clang
would generate a warning for each instance where we weren't returning
anything in a function that was meant to return a value.

    warning: non-void function does not return a value [-Wreturn-type]

For a specific example, my_radio::time_on_air() is supposed to return a
uint32_t, but wasn't returning anything. We'll return a zero instead of
relying on undefined behavior.

Without this, clang 11.0.1 was generating a virtual function
implementation with a `ud2` instruction to abort at run-time, causing
some execution of some unit tests to abort.

    Running main() from gmock_main.cc
    [==========] Running 10 tests from 1 test suite.
    [----------] Global test environment set-up.
    [----------] 10 tests from Test_LoRaPHYUS915
    [ RUN      ] Test_LoRaPHYUS915.constructor
    [       OK ] Test_LoRaPHYUS915.constructor (0 ms)
    [ RUN      ] Test_LoRaPHYUS915.restore_default_channels
    [       OK ] Test_LoRaPHYUS915.restore_default_channels (0 ms)
    [ RUN      ] Test_LoRaPHYUS915.rx_config
    [       OK ] Test_LoRaPHYUS915.rx_config (0 ms)
    [ RUN      ] Test_LoRaPHYUS915.tx_config
    Process 35669 stopped
    * thread ARMmbed#1, name = 'lorawan-loraphy-', stop reason = signal SIGILL: privileged instruction
        frame #0: 0x0000000000276f73 lorawan-loraphy-us915-unittest`my_radio::time_on_air(this=0x0000000800c2b048, modem=MODEM_LORA, pkt_len='\0') at Test_LoRaPHYUS915.cpp:90:5
       87       };
       88
       89       virtual uint32_t time_on_air(radio_modems_t modem, uint8_t pkt_len)
    -> 90       {
       91       };
       92
       93       virtual bool perform_carrier_sense(radio_modems_t modem,
    (lldb) disassemble --pc
    lorawan-loraphy-us915-unittest`my_radio::time_on_air:
    ->  0x276f73 <+67>: ud2
        0x276f75:       int3
        0x276f76:       int3
        0x276f77:       int3
    (lldb)
Remove two expressions from at_cellularcontexttest that do nothing. This
fixes the following two warnings.

    connectivity/cellular/tests/UNITTESTS/framework/AT/at_cellularcontext/at_cellularcontexttest.cpp:61:67: warning: expression result unused [-Wunused-value]            ATHandler_stub::int_valid_count_table[kRead_int_table_size];                                                                                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~^
    connectivity/cellular/tests/UNITTESTS/framework/AT/at_cellularcontext/at_cellularcontexttest.cpp:64:66: warning: expression result unused [-Wunused-value]            ATHandler_stub::read_string_table[kRead_string_table_size];
@Patater Patater added this to In progress (5) in Mbed Core via automation May 27, 2021
@Patater Patater moved this from In progress (5) to Reviewer approved & awaiting CI & merge (3) in Mbed Core May 27, 2021
@Patater
Copy link
Contributor Author

Patater commented May 27, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented May 27, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️

@0xc0170 0xc0170 merged commit bc7fc2b into ARMmbed:master May 31, 2021
Mbed Core automation moved this from Reviewer approved & awaiting CI & merge (3) to Done May 31, 2021
@mergify mergify bot removed the ready for merge label May 31, 2021
@mbedmain mbedmain added release-version: 6.12.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Mbed Core
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants