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

Uninitialized variable warning in UARTSerial at -O3 #6611

Merged
merged 1 commit into from Apr 23, 2018

Conversation

Projects
None yet
6 participants
@pauluap

pauluap commented Apr 11, 2018

Description

Appears when complied with -O3 optimization level

Compile: UARTSerial.cpp
../drivers/UARTSerial.cpp: In member function 'void mbed::UARTSerial::tx_irq()':
../drivers/UARTSerial.cpp:314:31: warning: 'data' may be used uninitialized in this function [-Wmaybe-uninitialized]
         SerialBase::_base_putc(data);

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@pauluap pauluap force-pushed the pauluap:compiler_warning_UARTSerial branch from 690c8a4 to a75201e Apr 12, 2018

@pauluap pauluap changed the base branch from mbed-os-5.8 to master Apr 12, 2018

@@ -305,11 +305,11 @@ void UARTSerial::rx_irq(void)
void UARTSerial::tx_irq(void)
{
bool was_full = _txbuf.full();
char data = 0;

This comment has been minimized.

@TeroJaasko

TeroJaasko Apr 16, 2018

Contributor

Isn't this now just masking the problem, which the compiler correctly warned for: if "CricularBuffer::pop()" returns false, the data -variable should not be forwarded to SerialBase::_base_putc()?

This comment has been minimized.

@pauluap

pauluap Apr 16, 2018

Unless there's threading issues involved here, I don't think that pop() will return false because of the check to empty() The compiler is not able to make that rationalization, so I'm not sure if testing the return value of pop() would help.

I suppose that it's writable as

    char data;

    /* Write to the peripheral if there is something to write
     * and if the peripheral is available to write. */
    while (SerialBase::writeable() && _txbuf.pop(data)) {
        SerialBase::_base_putc(data);
    }

Huh, it seems that the compiler was able to analyze the check to pop() after all, with that change I don't get a warning.

I switched the conditional so that it short-circuits the call to pop() if the object isn't writable.

I would go either way, within an IRQ I think that it's safe to call pop after !empty I chose the path of minimal change but could switch.

pop calls empty internally, so the end result is the same.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 16, 2018

Contributor

I think this version in the comment without the empty check is better. I think it's only the way it is because it's was imitating the read case.

@0xc0170 0xc0170 added needs: review and removed do not merge labels Apr 16, 2018

@0xc0170 0xc0170 requested a review from kjbracey-arm Apr 16, 2018

Paul Thompson
Eliminate complier warning and remove superfluous call to empty()
Appears when complied with -O3 optimization level

Compile: UARTSerial.cpp
../drivers/UARTSerial.cpp: In member function 'void mbed::UARTSerial::tx_irq()':
../drivers/UARTSerial.cpp:314:31: warning: 'data' may be used uninitialized in this function [-Wmaybe-uninitialized]
         SerialBase::_base_putc(data);

@pauluap pauluap force-pushed the pauluap:compiler_warning_UARTSerial branch from a75201e to d6c5f16 Apr 16, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

Starting CI since it looks like most recent commit addresses @kjbracey-arm' feedback.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 17, 2018

Build : SUCCESS

Build number : 1765
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6611/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@kjbracey-arm

Looks good to me. Pondered performance a bit - this does mean one more serial_writable call per IRQ if we're not saturating the serial port, but does significantly decrease the number of circular buffer calls (which are a bit clunky with IRQ enable/disable). Should be a net performance win when highly loaded, I hope.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

/morph export-build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 17, 2018

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

/morph mbed2-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

Attempting the close/reopen trick.

@cmonr cmonr closed this Apr 17, 2018

@cmonr cmonr reopened this Apr 17, 2018

@cmonr cmonr removed the needs: CI label Apr 17, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 17, 2018

Build : SUCCESS

Build number : 1779
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6611/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

1 similar comment
@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 19, 2018

/morph export-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 19, 2018

/morph mbed2-build

@0xc0170 0xc0170 added the needs: CI label Apr 19, 2018

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 20, 2018

@cmonr cmonr merged commit f372bcb into ARMmbed:master Apr 23, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10184 cycles (+656 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 23, 2018

@pauluap pauluap deleted the pauluap:compiler_warning_UARTSerial branch Apr 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment