bpo-37077: Add native thread ID (TID) for AIX

[aixtools]

This is the followup for issue36084

https://bugs.python.org/issue37077

[aixtools]

@jaketesler - Hi, and thanks in advance. Many thanks for offering to be "tagged"!

This seems straight forward - and I hope complete (docs updated). (also thanks to @ncoghlan iirc who pointed at thread_self() - the "kernel" routine versus the posix thread *_self routine!

[tiran]

-1

As you already said, pthread_self does not return the TID, https://linux.die.net/man/3/pthread_self

The thread ID returned by pthread_self() is not the same thing as the kernel thread ID returned by a call to gettid(2).

I'd rather not provide the feature at all than to provide an implementation that does not match with other implementations.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[aixtools]

-1

As you already said, pthread_self does not return the TID, https://linux.die.net/man/3/pthread_self

The thread ID returned by pthread_self() is not the same thing as the kernel thread ID returned by a call to gettid(2).

I'd rather not provide the feature at all than to provide an implementation that does not match with other implementations.

And I did not use pthread_self() - I used thread_self() - also a routine that has been around for years.

A short example - with also the verification from "ps" that the value returned by thread_self is - indeed - the KERNEL tid and not the POSIX_tid.

root@x066:[/data/prj/aixtools/tests/posix]./thread_self
thread_self: pid:7602402 tid:12779547 ptid:1
[1] + Stopped (SIGTSTP)        ./thread_self
root@x066:[/data/prj/aixtools/tests/posix]ps -p 7602402 -mo THREAD
    USER     PID    PPID       TID ST  CP PRI SC    WCHAN        F     TT BND COMMAND
    root 7602402 7209084         - T    0  60  1 f1000a040034adb0   200011  pts/0   - ./thread_self
       -       -       -  12779547 T    0  60  1 f1000a040034adb0   410400      -   - -
root@x066:[/data/prj/aixtools/tests/posix]cat thread_self.c
#include <pthread.h>
#include <sys/thread.h>
#include <stdio.h>

main()
{
        pid_t pid;
        tid_t tid;
        pthread_t ptid;

        pid = getpid();
        tid = thread_self();
        ptid = pthread_self();

        fprintf(stderr,"thread_self: pid:%d tid:%d ptid:%d\n", pid, tid, ptid);
        sleep(300);     /* give time to run ps -mo THREAD */
}

So, not understanding the GIT process - you requested a change - but I do not see anything to change. Other than perhaps my comment where I thanked 'someone' for pointing out the issue BEFORE I submitted anything.

Regards,
Michael

[jaketesler]

This may provide some clarity – https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.basetrf2/thread_self.htm

review was based on misreading the code

[ncoghlan]

@aixtools @encukou has cleared out the review that was based on misreading the update. It's likely worth adding a clarifying comment to the new AIX code, referencing the link that @jaketesler's provided, as I doubt @tiran will be the only reader to ever miss the fact that there's no leading "p" on the function name.

Do you have any thoughts on how we might be able to properly test this change? The existing native thread ID code doesn't appear to have any obvious tests either, so I wouldn't consider that a merge blocker, but it would still be nice if we could add that missing test coverage while folks have eyes on the code anyway.

[aixtools]

On 09/06/2019 07:38, Nick Coghlan wrote: @aixtools @encukou has cleared out the review that was based on misreading the update. It's likely worth adding a comment to the new AIX code, referencing the link that @jaketesler's provided. Do you have any thoughts on how we might be able to properly test this change? The existing native thread ID code doesn't appear to have any obvious tests either, so I wouldn't consider that a merge blocker, but it would still be nice if we could add that missing test coverage while folks have eyes on the code anyway.

With all mails coming from "Nicole Trudeau", sometime hard to know who is saying what. a) I have no issue with adding a reference to https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.basetrf2/thread_self.htm, but I wonder why to make an exception for the AIX bit. I had replied directly to "whoever" had made the review - that I had found the same comment in a book (white books I called them back then) on AIX 4.1 documentation from June 1995). I'll bow to your wisdom - but it feels funny giving AIX "special attention". iirc, the other platforms do not have any link re: their documentation. b) as to how to verify the the difference between "pthread"_tid() - whatever it name and "kernel"_tid() - for AIX I can only come up with something (that would work on AIX, do not know how to implement it on other platforms) is to verify the current PID is the "owner" of the TID would be to call, and parse a command such as "ps -p $PID -mo tid". ps -p $PID # fairly common"

…

-o tid # I expect to be 'common' as well

-m (on AIX stands for multi-line), An example - not as root, so it works for all: ps -p $$ -mo tid       TID         -  22937633 So, while it could be used "as proof" - I think it would need to be a separate PR/issue - as it will require "platform" input for all platforms. As a footnote - I would not mind learning how to expand a test (even if only for AIX) to make this comparison - as long as you know, in advance, the basic approach I would take. Regards, Michael

[bedevere-bot]

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

[methane]

Yes, it looks a bit like a recommendation. FWIW, I recently had this conversation with a highly advanced git user, and his opinion was to merge master into the PR for all projects that squash merges, because the alternative is too complex for PRs that are open for a long time.

I agree with him. While I use rebase often (especially for branches before sending pull request), it requires high git knowledge. Additionally, it requires -f option when git push. It is dangerous for new git users too.
I think "git rebase" shouldn't be recommended for new users.

[aixtools]

I am thinking the "rebase" - read "merge" should be written up in the devguide. I tried reading up on it yesterday and this morning - and my head hurts - how to make it simple.

What I think has been vital for me was seeing how to merge from "upstream". I thought I had to maintain my fork at 100% - so I had a separate directory for "maintaining" the fork and then did all my cloneing, merge, pull, etc. from my fork rather than from "upstream".

I suppose when in a team using git - rather than "solo" (aka wannabe 'member') getting advice and direction in how to use git effectively is more efficient than googling through multiple opinions written up as tutorials.

:) so happy this "merge" went smoothly.

[methane]

I sent pull request to stop recommend "rebase": python/devguide#496

[asvetlov]

Suggested change

	Add native thread ID (TID) for AIX
	:func:`threading.get_native_id` now also supports AIX.

[asvetlov]

Suggested change

	.. availability:: Windows, FreeBSD, Linux, macOS, OpenBSD, NetBSD, AIX.
	.. availability:: Requires :func:`get_native_id` function.

Please keep the text from master.
@vstinner approved the change for NetBSD, I think we don't have to change it again (and fix doc in two places once next implementation will be added).

[jaketesler]

“Require” or “Requires”?

[asvetlov]

grepping python doc provides the only similarity from os.rst:

.. availability:: Unix, Windows.  :func:`spawnlp`, :func:`spawnlpe`, :func:`spawnvp`

Also doesn't look perfect.

I don't know, I'm not a native speaker. Ok with any decision.

[jaketesler]

I’d suggest “Requires.” Maybe this will set a good precedent. ¯_(ツ)_/¯

[asvetlov]

Updated suggestion

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[aixtools]

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

[vstinner]

LGTM.

[miss-islington]

Thanks @aixtools for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[bedevere-bot]

GH-14067 is a backport of this pull request to the 3.8 branch.