Skip to content

Give threads a recognisable name to aid in debugging#1515

Merged
gavinandresen merged 5 commits intobitcoin:masterfrom
muggenhor:named-threads
Jul 17, 2012
Merged

Give threads a recognisable name to aid in debugging#1515
gavinandresen merged 5 commits intobitcoin:masterfrom
muggenhor:named-threads

Conversation

@muggenhor
Copy link
Copy Markdown
Contributor

These thread names are visible in gdb when using 'info threads'. Additionally both 'top' and 'ps' show these names unless told to display the command-line instead of task name.

@sipa
Copy link
Copy Markdown
Member

sipa commented Jun 24, 2012

ACK

@gavinandresen
Copy link
Copy Markdown
Contributor

Good idea.

@Diapolo
Copy link
Copy Markdown

Diapolo commented Jun 24, 2012

Sounds good.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Jun 25, 2012

Good idea.

I've only got a comment about the thread names: it makes sense to link them to thread names in in the enumeration in net.h (sockethandler, openconnections, messagehandler, miner, etc...) to be consistent and for easy debugging. Or add least add a comment in net.h by which name each of the threads can be recognized.

@muggenhor
Copy link
Copy Markdown
Contributor Author

@laanwj I've tried to make the names look like those in the thread enumeration, unfortunately only 15 characters are available. So I've deviated from those names for some instances to improve readability. That being said I could add a copy of the thread names as comments to that enum.

Comment thread src/util.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason to check for linux specifically?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That syscall with that parameter is Linux specific, additionally the PR_SET_NAME checking-part is because it's only available on Linux upwards of 2.6.9.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, my point was that just checking for PR_SET_NAME should probably suffice?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, I suppose it does, removed the linux part.

@luke-jr
Copy link
Copy Markdown
Member

luke-jr commented Jun 28, 2012

Might add pthread_set_name_np for BSD.

I haven't checked, but it would be annoying if this erases the commandline parameters - I use a dummy -name=foo to discern between different bitcoinds in ps right now.

@muggenhor
Copy link
Copy Markdown
Contributor Author

@luke-jr If pthread_set_name_np is implemented similar to BSD's setprogname then it will modify the commandline parameters (that's how it does this: overwriting argv[0-n] memory).

It's manpage seems to suggest otherwise though. I can add the code easily enough, but I'm not even sure if my FreeBSD 8 installation still boots. So I'd appreciate if you could test & confirm that it works as advertised,

@luke-jr
Copy link
Copy Markdown
Member

luke-jr commented Jun 30, 2012

@muggenhor I only use Linux, just came across the BSD variant looking at your pullreq.

@muggenhor
Copy link
Copy Markdown
Contributor Author

@luke-jr Right, I've just confirmed that I can cross-compile an isolated call to pthread_set_name_np for FreeBSD 8.

I'll try dusting off my FreeBSD 8 box for a test run.

@muggenhor
Copy link
Copy Markdown
Contributor Author

Right, my FreeBSD system doesn't even boot properly anymore. So I'll disable the FreeBSD code (but leave it in) so that it won't break on those systems but another developer (who does have FreeBSD/OpenBSD) can confirm that it does work and enable it.

Comment thread src/util.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please move this implementation (and associated includes such as sys/prctl.h) to the .cpp instead of the .h, there is no need for inlining here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ACK, see rebased commits.

@Diapolo
Copy link
Copy Markdown

Diapolo commented Jul 8, 2012

In qtipcserver.cpp we have another thread afaik, any reason to not cover that one in this pull?

@muggenhor
Copy link
Copy Markdown
Contributor Author

@Diapolo: that was just an oversight. Addressed in my last commit.

NOTE: These thread names are visible in gdb when using 'info threads'.
      Additionally both 'top' and 'ps' show these names *unless* told to
      display the command-line instead of task name.

Signed-off-by: Giel van Schijndel <me@mortis.eu>
 * Fix wrong thread name for wallet *relocking* thread
  - Was named the unlocking thread
 * Use consistent naming

Signed-off-by: Giel van Schijndel <me@mortis.eu>
NOTE: This is currently disabled, until a developer with FreeBSD/OpenBSD
      can confirm that this works (without causing undefined behaviour
      preferrably).

Signed-off-by: Giel van Schijndel <me@mortis.eu>
… instead

Signed-off-by: Giel van Schijndel <me@mortis.eu>
Signed-off-by: Giel van Schijndel <me@mortis.eu>
@muggenhor
Copy link
Copy Markdown
Contributor Author

Rebased again, still waiting to be merged...

@gavinandresen gavinandresen merged commit 36fe965 into bitcoin:master Jul 17, 2012
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
* Make ProcessNewBlock dbp const and update comment

* Switch reindexing to AcceptBlock in-loop and ActivateBestChain afterwards

* Optimize ActivateBestChain for long chains

* Add -reindex-chainstate that does not rebuild block index

* Report reindexing progress in GUI
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 22, 2019
* message processing: Allow filtersizext and sendheaders during initial handshake

* message processing: Allow buversion early in initial handshake

This is to fix the following bug in the xversion handling that
occured with a xversion enabled node (latest release candidate
at the time of this writing) talking to a BUCash 1.5.0.2 node:

2018-12-05 16:08:20 received: version (118 bytes) peer=localhost:8432 (403)
2018-12-05 16:08:20 sending msg: verack (0 bytes) peer=403
2018-12-05 16:08:20 receive version message: /BUCash:1.5.0.2(SV; EB32; AD12)/: version 80003, blocks=559512, us=0.0.0.0:0, peer=localhost:8432 (403)
2018-12-05 16:08:20 received: sendheaders (0 bytes) peer=localhost:8432 (403)
2018-12-05 16:08:20 This node does not support XVERSION as it did not send it at the right time but answered with sendheaders instead. peer=localhost:8432 (403) version=/BUCash:1.5.0.2(SV; EB32; AD12)/
2018-12-05 16:08:20 sending msg: getaddr (0 bytes) peer=403
2018-12-05 16:08:20 sending msg: sendheaders (0 bytes) peer=403
2018-12-05 16:08:20 Ignoring command sendheaders that comes in before initial handshake is finished. peer=localhost:8432 (403) version=/BUCash:1.5.0.2(SV; EB32; AD12)/
2018-12-05 16:08:20 ProcessMessages(sendheaders, 0 bytes) FAILED peer localhost:8432 (403)
2018-12-05 16:08:20 received: filtersizext (4 bytes) peer=localhost:8432 (403)
2018-12-05 16:08:20 Ignoring command filtersizext that comes in before initial handshake is finished. peer=localhost:8432 (403) version=/BUCash:1.5.0.2(SV; EB32; AD12)/
2018-12-05 16:08:20 ProcessMessages(filtersizext, 4 bytes) FAILED peer localhost:8432 (403)
2018-12-05 16:08:20 received: buversion (2 bytes) peer=localhost:8432 (403)
2018-12-05 16:08:20 Misbehaving: localhost:8432 (403) (0 -> 100) BAN THRESHOLD EXCEEDED
2018-12-05 16:08:20 ERROR: buversion message received unexpectedly from peer=localhost:8432 (403) version=/BUCash:1.5.0.2(SV; EB32; AD12)/ state_incoming=READY state_outgoing=SENT_VERSION
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request May 6, 2020
a60b7cc qt: Replace objc_msgSend with native syntax (furszy)
5d6f9b5 qt: Use GUIUtil::bringToFront where possible (furszy)
9e85183 qt: Add GUIUtil::bringToFront (furszy)
188199a Remove obj_c for macOS Dock icon menu (furszy)
75a41e2 Use Qt signal for macOS Dock icon click event (furszy)
ff319ad Remove obj_c for macOS Dock icon setting. (furszy)

Pull request description:

  Updating `MacDockIconHandler` to latest upstream. Fixing bitcoin#1512 .

  Back ported PRs:

  #### gui: Add GUIUtil::bringToFront [14123](bitcoin#14123):
  ```
  The sequence show -> raise -> activateWindow is factored out to the new function GUIUtil::bringToFront where a macOS fix is added in order to fix bitcoin#13829.

  Also included:

  simplification of BitcoinGUI::showNormalIfMinimized
  ```

  #### qt: Replace objc_msgSend() function calls with the native Objective-C syntax [16720](bitcoin#16720):

  ```
  Changes in Xcode 11 Objective-C Runtime cause an error (bitcoin#16387) during building on MacOS 10.15 Catalina.

  This PR fixes this issue by replacing objc_msgSend() function calls with the native Objective-C syntax.

  Refs:

  changes in objc_msgSend function
  OBJC_OLD_DISPATCH_PROTOTYPES macro
  ```

ACKs for top commit:
  random-zebra:
    utACK a60b7cc
  Fuzzbawls:
    ACK a60b7cc

Tree-SHA512: 95543a12d58187f17d27c6757351becd43a92c3edb8fcbc0a2b03a54d31cbad604ae149f2e866d82c3ce5f9a636571b8e51137fc5f639031a20080d8290889d9
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants