Add attribute [[noreturn]] (C++11) to functions that will not return #10843

practicalswift · 2017-07-16T13:07:27Z

Add attribute [[noreturn]] (C++11) to functions that will not return.

Rationale:

Reduce the number of false positives/false negatives from static analyzers with regards to things such as unused or unreachable code
Potentially enable additional compiler optimizations

TheBlueMatt · 2017-07-16T18:39:06Z

src/random.cpp

@@ -39,7 +39,7 @@
 #include <openssl/err.h>
 #include <openssl/rand.h>

-static void RandFailure()
+[[noreturn]] static void RandFailure()
 {
    LogPrintf("Failed to read randomness, aborting\n");
    abort();


Maybe change this to std::abort() so that its clearer that we're really [[noreturn]]ing?

Good point! Fixed! :-)

sipa · 2017-07-16T18:57:50Z

utACK 2e737cf996336fbf74ceb56d3f105fd7fcc13a3b

gmaxwell

utACK

paveljanik · 2017-07-17T08:56:23Z

ACK 2e737cf

laanwj · 2017-07-17T13:03:29Z

The PR description mention 'functions' where this changes only one function. Is that intentional, or did you miss something while pushing?

gmaxwell · 2017-07-17T13:22:16Z

There were more before...

practicalswift · 2017-07-17T13:27:50Z

@laanwj Yes, originally I aimed to add the [[noreturn]] attribute also to:

BitcoinApplication::handleRunawayException(const QString &message) (in qt/bitcoin.cpp)
Shutdown()/StartShutdown() (in test/test_bitcoin_main.cpp)

But I ran into some compilation issues that I didn't have time to follow up on, so I excluded them from this PR.

TheBlueMatt · 2017-07-17T15:11:53Z

src/qt/bitcoin.cpp

@@ -519,7 +519,7 @@ void BitcoinApplication::shutdownResult()
    quit(); // Exit main loop after shutdown finished
 }

-void BitcoinApplication::handleRunawayException(const QString &message)
+[[noreturn]] void BitcoinApplication::handleRunawayException(const QString &message)
 {
    QMessageBox::critical(0, "Runaway exception", BitcoinGUI::tr("A fatal error occurred. Bitcoin can no longer continue safely and will quit.") + QString("\n\n") + message);
    ::exit(EXIT_FAILURE);


Rationale: * Reduce the number of false positives from static analyzers * Potentially enable additional compiler optimizations

practicalswift · 2017-07-17T16:55:26Z

I've now added the [[noreturn]] attribute to Shutdown()/StartShutdown() (in test/test_bitcoin_main.cpp).

However, after some digging it appears that [[noreturn]] might not be correct for BitcoinApplication::handleRunawayException(const QString &message). If I understand it correctly it calls QCoreApplication::exit(…) which does return to the caller (it is event processing that stops).

This PR should now be ready for review.

laanwj · 2017-07-17T18:12:16Z

If I understand it correctly it calls QCoreApplication::exit(…) which does return to the caller (it is event processing that stops).

Agreed - if you're not 100% sure that a function doesn't return, it's much better to err on the safe side and not specify it. From experience, functions with noreturn returning can result in some awful things, which might even be security critical (but at the least hard to debug), as it can fall through to apparently random code.

TheBlueMatt · 2017-07-17T19:19:09Z

@practicalswift I'm confused, then, BitcoinApplication::handleRunawayException ends by calling ::exit (the global exit, not QCoreApplication::exit, no?)?

practicalswift · 2017-07-17T23:48:07Z

@TheBlueMatt You're absolutely right! I was wrong about QCoreApplication::exit(…) being called. Sorry :-)

After applying 536504c8bef776902fa071dd76a37ef7037c1945 make check passes for all build configurations except HOST=x86_64-apple-darwin11, see https://travis-ci.org/bitcoin/bitcoin/builds/254651379. I don't understand why that is.

theuni · 2017-07-18T00:57:17Z

The attribute needs to be on the declaration rather than the definition.

practicalswift · 2017-07-18T03:58:00Z

@theuni Yes, I thought so but when applying d52e345a6e7526f516425b7547dbca534a884172 I got the parsing error qt/bitcoin.cpp:[…]: Parse error at ";" for all Qt-enabled builds, see https://travis-ci.org/bitcoin/bitcoin/jobs/254702626#L2290.

$ make V=1
[…]
QT_SELECT=qt5 /usr/lib/x86_64-linux-gnu/qt5/bin/moc -I. -I../src/config -I/usr/include/x86_64-linux-gnu/qt5/QtNetwork -I/usr/include/x86_64-linux-gnu/qt5 -I/usr/include/x86_64-linux-gnu/qt5/QtWidgets -I/usr/include/x86_64-linux-gnu/qt5 -I/usr/include/x86_64-linux-gnu/qt5/QtGui -I/usr/include/x86_64-linux-gnu/qt5 -I/usr/include/x86_64-linux-gnu/qt5/QtCore -I/usr/include/x86_64-linux-gnu/qt5 -DHAVE_CONFIG_H -I. qt/bitcoin.cpp | \
  /bin/sed -e '/^\*\*.*Created:/d' -e '/^\*\*.*by:/d' > qt/bitcoin.moc
qt/bitcoin.cpp:233: Parse error at ";"
[…]
$ wc -c src/qt/bitcoin.moc
0 src/qt/bitcoin.moc
$ /usr/lib/x86_64-linux-gnu/qt5/bin/moc --version
moc 5.5.1

Qt:s moc being confused by the [[noreturn]] attribute? :-)

practicalswift · 2017-07-18T15:13:02Z

I've now excluded the commit touching handleRunawayException(…) (src/qt/bitcoin.cpp). Qt's moc is doing too much stuff behind the scenes for me to be comfortable doing that change :-)

This PR should now be ready for review.

TheBlueMatt · 2017-07-19T20:47:44Z

utACK b82c55a

…that will not return b82c55a Add attribute [[noreturn]] (C++11) to functions that will not return (practicalswift) Pull request description: Add attribute `[[noreturn]]` (C++11) to functions that will not return. Rationale: * Reduce the number of false positives/false negatives from static analyzers with regards to things such as unused or unreachable code * Potentially enable additional compiler optimizations Tree-SHA512: 899683fe8b2fcf19bd334352271d368b46b805be9d426aac1808335fd95732d6d7078d3296951b9879196f3f6e3ec0fdb7695d0afdc3fbe4dd78a2ca70e91ff7 Signed-off-by: Pasta <pasta@dashboost.org> # Conflicts: # src/test/test_bitcoin_main.cpp

…that will not return b82c55a Add attribute [[noreturn]] (C++11) to functions that will not return (practicalswift) Pull request description: Add attribute `[[noreturn]]` (C++11) to functions that will not return. Rationale: * Reduce the number of false positives/false negatives from static analyzers with regards to things such as unused or unreachable code * Potentially enable additional compiler optimizations Tree-SHA512: 899683fe8b2fcf19bd334352271d368b46b805be9d426aac1808335fd95732d6d7078d3296951b9879196f3f6e3ec0fdb7695d0afdc3fbe4dd78a2ca70e91ff7 Signed-off-by: Pasta <pasta@dashboost.org> Fixes for bitcoin#10843 backport

…that will not return b82c55a Add attribute [[noreturn]] (C++11) to functions that will not return (practicalswift) Pull request description: Add attribute `[[noreturn]]` (C++11) to functions that will not return. Rationale: * Reduce the number of false positives/false negatives from static analyzers with regards to things such as unused or unreachable code * Potentially enable additional compiler optimizations Tree-SHA512: 899683fe8b2fcf19bd334352271d368b46b805be9d426aac1808335fd95732d6d7078d3296951b9879196f3f6e3ec0fdb7695d0afdc3fbe4dd78a2ca70e91ff7 Signed-off-by: Pasta <pasta@dashboost.org> Fixes for bitcoin#10843 backport 10843 continued Signed-off-by: Pasta <pasta@dashboost.org>

003929c refactor: add [[noreturn]] attribute where applicable (fanquake) Pull request description: Similar to #10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely. ACKs for top commit: vasild: ACK 003929c Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705

…cable 003929c refactor: add [[noreturn]] attribute where applicable (fanquake) Pull request description: Similar to bitcoin#10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely. ACKs for top commit: vasild: ACK 003929c Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705

…that will not return b82c55a Add attribute [[noreturn]] (C++11) to functions that will not return (practicalswift) Pull request description: Add attribute `[[noreturn]]` (C++11) to functions that will not return. Rationale: * Reduce the number of false positives/false negatives from static analyzers with regards to things such as unused or unreachable code * Potentially enable additional compiler optimizations Tree-SHA512: 899683fe8b2fcf19bd334352271d368b46b805be9d426aac1808335fd95732d6d7078d3296951b9879196f3f6e3ec0fdb7695d0afdc3fbe4dd78a2ca70e91ff7 Signed-off-by: Pasta <pasta@dashboost.org> Fixes for bitcoin#10843 backport 10843 continued Signed-off-by: Pasta <pasta@dashboost.org>

practicalswift mentioned this pull request Jul 16, 2017

Fix resource leak on error in GetDevURandom #10837

Merged

practicalswift force-pushed the noreturn branch 3 times, most recently from c4d1553 to 157c2d5 Compare July 16, 2017 14:25

TheBlueMatt reviewed Jul 16, 2017

View reviewed changes

practicalswift force-pushed the noreturn branch from 157c2d5 to 2e737cf Compare July 16, 2017 18:54

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

practicalswift force-pushed the noreturn branch from 2e737cf to b40017f Compare July 17, 2017 14:15

TheBlueMatt reviewed Jul 17, 2017

View reviewed changes

Add attribute [[noreturn]] (C++11) to functions that will not return

b82c55a

Rationale: * Reduce the number of false positives from static analyzers * Potentially enable additional compiler optimizations

practicalswift force-pushed the noreturn branch from b40017f to b82c55a Compare July 17, 2017 16:47

practicalswift force-pushed the noreturn branch 3 times, most recently from c340a51 to 536504c Compare July 17, 2017 23:05

practicalswift force-pushed the noreturn branch from 536504c to d52e345 Compare July 18, 2017 03:30

practicalswift force-pushed the noreturn branch from d52e345 to b82c55a Compare July 18, 2017 15:10

kallewoof approved these changes Aug 10, 2017

View reviewed changes

codablock added a commit to codablock/dash that referenced this pull request Dec 31, 2019

Fixes for bitcoin#10843 backport

fbe7be5

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 1, 2020

Fixes for bitcoin#10843 backport

2c6b445

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 1, 2020

Fixes for bitcoin#10843 backport

cfb403e

UdjinM6 mentioned this pull request Jan 2, 2020

Backports 0.16 pr1-4: other dashpay/dash#3106

Merged

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 2, 2020

Fixes for bitcoin#10843 backport

8e0a02f

fanquake mentioned this pull request Apr 7, 2021

refactor: add [[noreturn]] attribute where applicable #21633

Merged

practicalswift deleted the noreturn branch April 10, 2021 19:32

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add attribute [[noreturn]] (C++11) to functions that will not return #10843

Add attribute [[noreturn]] (C++11) to functions that will not return #10843

practicalswift commented Jul 16, 2017 •

edited

TheBlueMatt Jul 16, 2017

practicalswift Jul 16, 2017

sipa commented Jul 16, 2017

gmaxwell left a comment

paveljanik commented Jul 17, 2017

laanwj commented Jul 17, 2017

gmaxwell commented Jul 17, 2017

practicalswift commented Jul 17, 2017

TheBlueMatt Jul 17, 2017

practicalswift commented Jul 17, 2017

laanwj commented Jul 17, 2017 •

edited

TheBlueMatt commented Jul 17, 2017

practicalswift commented Jul 17, 2017 •

edited

theuni commented Jul 18, 2017

practicalswift commented Jul 18, 2017 •

edited

practicalswift commented Jul 18, 2017 •

edited

TheBlueMatt commented Jul 19, 2017

Add attribute [[noreturn]] (C++11) to functions that will not return #10843

Add attribute [[noreturn]] (C++11) to functions that will not return #10843

Conversation

practicalswift commented Jul 16, 2017 • edited

TheBlueMatt Jul 16, 2017

Choose a reason for hiding this comment

practicalswift Jul 16, 2017

Choose a reason for hiding this comment

sipa commented Jul 16, 2017

gmaxwell left a comment

Choose a reason for hiding this comment

paveljanik commented Jul 17, 2017

laanwj commented Jul 17, 2017

gmaxwell commented Jul 17, 2017

practicalswift commented Jul 17, 2017

TheBlueMatt Jul 17, 2017

Choose a reason for hiding this comment

practicalswift commented Jul 17, 2017

laanwj commented Jul 17, 2017 • edited

TheBlueMatt commented Jul 17, 2017

practicalswift commented Jul 17, 2017 • edited

theuni commented Jul 18, 2017

practicalswift commented Jul 18, 2017 • edited

practicalswift commented Jul 18, 2017 • edited

TheBlueMatt commented Jul 19, 2017

practicalswift commented Jul 16, 2017 •

edited

laanwj commented Jul 17, 2017 •

edited

practicalswift commented Jul 17, 2017 •

edited

practicalswift commented Jul 18, 2017 •

edited

practicalswift commented Jul 18, 2017 •

edited