refactor: Replace global find_value function with UniValue::find_value method #27605

maflcko · 2023-05-09T07:52:05Z

The global function has issues:

It causes gcc-13 warnings, see GCC 13: -Wdangling-reference output #26926
There is no rationale for it being a global function, when it acts like a member function
performance-unnecessary-copy-initialization clang-tidy isn't run on it

Fix all issues by making it a member function.

DrahtBot · 2023-05-09T07:52:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, achow101
Concept ACK	fanquake
Stale ACK	stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake · 2023-05-09T09:49:20Z

Concept ACK

hebasto · 2023-05-09T09:58:27Z

Concept ACK.

hebasto

ACK fa0d180f48d4e5253f58aced32768369f963d1c7, tested on Ubuntu 23.04 using GCC 13.0.1.

maflcko · 2023-05-09T10:51:08Z

Ubuntu 23.04 using GCC 13.0.1

I think it ships with 12.2? https://packages.ubuntu.com/lunar/gcc

stickies-v

ACK fa0d180f48d4e5253f58aced32768369f963d1c7

I can't see any behaviour change, and as far as I can tell using references in fa0d180f48d4e5253f58aced32768369f963d1c7 doesn't introduce any (potential) lifetime issues.

stickies-v · 2023-05-09T10:42:35Z

src/univalue/include/univalue.h

I think this should be LIFETIMEBOUND?

Maybe in a follow-up for the whole univalue interface?

hebasto · 2023-05-09T13:08:56Z

Ubuntu 23.04 using GCC 13.0.1

I think it ships with 12.2? packages.ubuntu.com/lunar/gcc

https://packages.ubuntu.com/lunar/g++-13 has been installed as sudo apt install g++-13.

fanquake · 2023-05-09T15:43:33Z

With fa0d180f48d4e5253f58aced32768369f963d1c7 & gcc (GCC) 13.1.1 20230426 (Red Hat 13.1.1-1), fixes all issues from #26926 except for:

test/interfaces_tests.cpp: In member function ‘void interfaces_tests::findCommonAncestor::test_method()’:
test/interfaces_tests.cpp:101:19: warning: possibly dangling reference to a temporary [-Wdangling-reference]
  101 |     const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
      |                   ^~~~~~
In file included from ./common/args.h:9,
                 from ./test/util/setup_common.h:9,
                 from test/interfaces_tests.cpp:9:
./sync.h:302:96: note: the temporary was destroyed at the end of the full expression ‘<lambda closure object>interfaces_tests::findCommonAncestor::test_method()::<lambda()>{((interfaces_tests::findCommonAncestor*)this)}.interfaces_tests::findCommonAncestor::test_method()::<lambda()>()’
  302 | #define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }())
      |                                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
test/interfaces_tests.cpp:101:28: note: in expansion of macro ‘WITH_LOCK’
  101 |     const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
      |                            ^~~~~~~~~
  CXX      test/test_bitcoin-netbase_tests.o

-BEGIN VERIFY SCRIPT- sed --regexp-extended -i 's/find_value\(([^ ,]+), /\1.find_value(/g' $(git grep -l find_value) -END VERIFY SCRIPT-

This can be reverted once gcc excludes lambdas with decltype(auto) return type from its -Wdangling-reference analysis.

maflcko · 2023-05-09T18:28:55Z

except for ...

Might be good to ask the gcc devs if they want to fix this, as it seems a different false positive for decltype(auto) return type lambdas?

Still, added a temporary commit here to hack around it.

stickies-v · 2023-05-10T10:06:21Z

src/test/interfaces_tests.cpp

 {
    auto& chain = m_node.chain;
-    const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
+    const CChain& active{*WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return &Assert(m_node.chainman)->ActiveChain())};


I think it would be best to add the commit message as code documentation to make it easier to track when this can be reverted?

I'm also not sure this commit fits the scope of the PR but I can see how it's related, and it's a separate commit anyway so probably fine.

I am thinking that this may not be fixed by upstream gcc, and anyway, gcc-13.1.1 is released and will stick around, so this can't be reverted any time soon, anyway. So it might be best to just make it permanent now?

GCC 13 introduced a broken behavior, and we usually do not modify our correct code to make a broken compiler happy. Considering that this is the only line in test code, I lean to agree with this change.

As the test is executed in a single thread, is it a big deal to hold ::cs_main during the entire test case?

Why not to use the pattern as

bitcoin/src/test/interfaces_tests.cpp

Lines 89 to 91 in fa266c4

LOCK(Assert(m_node.chainman)->GetMutex());

auto& chain = m_node.chain;

const CChain& active = Assert(m_node.chainman)->ActiveChain();

or

bitcoin/src/test/interfaces_tests.cpp

Lines 130 to 132 in fa266c4

LOCK(::cs_main);

auto& chain = m_node.chain;

const CChain& active = Assert(m_node.chainman)->ActiveChain();

?

Likely due to a LOCKS_EXCLUDED?

Likely due to a LOCKS_EXCLUDED?

Right, in Chainstate::InvalidateBlock.

hebasto

re-ACK fa266c4

hebasto · 2023-05-10T11:28:16Z

Maybe notice the GCC-13 bug in the comments for #define WITH_LOCK?

maflcko · 2023-05-10T11:55:24Z

Maybe notice the GCC-13 bug in the comments for #define WITH_LOCK?

I think #27605 (comment) applies and we should disable the warning instead if there are more false positives?

achow101 · 2023-05-10T15:58:08Z

ACK fa266c4

…h UniVa…

…e method Summary: These warning are very noisy and annoying when working on large refactors, hiding the useful information from the developer. Backport of [[bitcoin/bitcoin#27605 | core#27605]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D15097

DrahtBot added the Refactoring label May 9, 2023

maflcko mentioned this pull request May 9, 2023

GCC 13: -Wdangling-reference output #26926

Closed

DrahtBot added the CI failed label May 9, 2023

DrahtBot removed the CI failed label May 9, 2023

hebasto approved these changes May 9, 2023

View reviewed changes

stickies-v approved these changes May 9, 2023

View reviewed changes

DrahtBot added the Needs rebase label May 9, 2023

MarcoFalke added 4 commits May 9, 2023 18:47

Add UniValue::find_value method

fa548ac

scripted-diff: Use UniValue::find_value method

fa422ae

-BEGIN VERIFY SCRIPT- sed --regexp-extended -i 's/find_value\(([^ ,]+), /\1.find_value(/g' $(git grep -l find_value) -END VERIFY SCRIPT-

Remove unused find_value global function

faaa60a

Fix clang-tidy performance-unnecessary-copy-initialization warnings

fa28850

maflcko force-pushed the 2305-univalue-no-dangling- branch from fa0d180 to fa28850 Compare May 9, 2023 16:49

Temporarily work around gcc-13 warning bug in interfaces_tests

fa266c4

This can be reverted once gcc excludes lambdas with decltype(auto) return type from its -Wdangling-reference analysis.

DrahtBot removed the Needs rebase label May 9, 2023

DrahtBot mentioned this pull request May 9, 2023

Support JSON-RPC 2.0 when requested by client #27101

Merged

fanquake requested a review from stickies-v May 10, 2023 09:50

stickies-v reviewed May 10, 2023

View reviewed changes

hebasto approved these changes May 10, 2023

View reviewed changes

DrahtBot requested a review from stickies-v May 10, 2023 11:23

achow101 merged commit e0a70c5 into bitcoin:master May 10, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 10, 2023

Merge bitcoin#27605: refactor: Replace global find_value function wit…

5b6561b

…h UniVa…

maflcko deleted the 2305-univalue-no-dangling- branch May 11, 2023 07:56

Sjors mentioned this pull request May 19, 2023

Improve display address handling for external signer #24313

Merged

bitcoin locked and limited conversation to collaborators May 10, 2024

	LOCK(Assert(m_node.chainman)->GetMutex());
	auto& chain = m_node.chain;
	const CChain& active = Assert(m_node.chainman)->ActiveChain();

	LOCK(::cs_main);
	auto& chain = m_node.chain;
	const CChain& active = Assert(m_node.chainman)->ActiveChain();

refactor: Replace global find_value function with UniValue::find_value method #27605

refactor: Replace global find_value function with UniValue::find_value method #27605

Uh oh!

Conversation

maflcko commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented May 9, 2023

Uh oh!

hebasto commented May 9, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 9, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v May 9, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko May 9, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented May 9, 2023

Uh oh!

maflcko commented May 9, 2023

Uh oh!

stickies-v May 10, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko May 10, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto May 10, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko May 10, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto May 10, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 10, 2023

Uh oh!

maflcko commented May 10, 2023

Uh oh!

achow101 commented May 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maflcko commented May 9, 2023 •

edited

Loading

DrahtBot commented May 9, 2023 •

edited

Loading

hebasto commented May 9, 2023 •

edited

Loading

hebasto May 10, 2023 •

edited

Loading