Skip to content

build: Specify AM_CPPFLAGS for ZMQ #16434

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Jul 30, 2019
Merged

build: Specify AM_CPPFLAGS for ZMQ #16434

laanwj merged 1 commit into from
Jul 30, 2019

Conversation

@domob1812
Copy link
Contributor

@domob1812 domob1812 commented Jul 22, 2019

When building the ZMQ static library, add AM_CPPFLAGS to the library CPPFLAGS. Otherwise, we may miss important flags that are specified elsewhere. For instance, if --enable-debug is passed and
-DDEBUG_LOCKORDER set, then that would not apply to the ZMQ library before (causing potential for hard-to-find bugs).

@domob1812
Specify AM_CPPFLAGS for ZMQ.
29ee4c4 
When building the ZMQ static library, add AM_CPPFLAGS to the library
CPPFLAGS.  Otherwise, we may miss important flags that are specified
elsewhere.  For instance, if --enable-debug is passed and
-DDEBUG_LOCKORDER set, then that would not apply to the ZMQ library
before (causing potential for hard-to-find bugs).
@domob1812
Copy link
Contributor Author

domob1812 commented Jul 22, 2019

While this seems to currently not cause any issues, it could do so in the future. For instance, I'm working on some patches applied on top of Bitcoin Core. That code previously had a bug where cs_main was not locked even though it should have been, but AssertLockHeld did not catch this. After 682a1d0, a different (but also wrong) behaviour showed instead: Now AssertLockHeld triggered, even though cs_main was locked correctly. Both of those bugs were caused because -DDEBUG_LOCKORDER was applied inconsistently between the ZMQ code and other parts of the code.

Note that even the unmodified ZMQ code does lock cs_main. So presumably if ReadBlockFromDisk were changed to AssertLockHeld(cs_main), then the same bug would actively occur in the vanilla codebase as well.

@fanquake fanquake requested a review from theuni July 28, 2019 23:57
@fanquake fanquake changed the title Specify AM_CPPFLAGS for ZMQ build: Specify AM_CPPFLAGS for ZMQ Jul 28, 2019
@laanwj
Copy link
Member

laanwj commented Jul 30, 2019

utACK 29ee4c4

pull bot pushed a commit to uniibu/bitcoin that referenced this pull request Jul 30, 2019
@laanwj
Merge bitcoin#16434: build: Specify AM_CPPFLAGS for ZMQ
bd35ec3 
29ee4c4 Specify AM_CPPFLAGS for ZMQ. (Daniel Kraft)

Pull request description:

  When building the ZMQ static library, add `AM_CPPFLAGS` to the library `CPPFLAGS`.  Otherwise, we may miss important flags that are specified elsewhere.  For instance, if `--enable-debug` is passed and
  `-DDEBUG_LOCKORDER` set, then that would not apply to the ZMQ library before (causing potential for hard-to-find bugs).

ACKs for top commit:
  laanwj:
    utACK 29ee4c4

Tree-SHA512: 64085d71ed3f435a6e4df6dc42bda8b6159a4d292d0547c5b38c09d6ac95e976ad1728cd65278bffdd57363f60a58eb762b1171dafbe055cf94ffcd4f66da877
@laanwj laanwj merged commit 29ee4c4 into bitcoin:master Jul 30, 2019
@domob1812 domob1812 deleted the zmq-cppflags branch July 30, 2019 14:07
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@theuni theuni Awaiting requested review from theuni

Assignees

No one assigned

Labels

Build system RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@domob1812 @laanwj @fanquake