Skip to content
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.

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

Possible violation of the pthread_create interface? #4324

Open
Dante-Broggi opened this issue Feb 21, 2023 · 4 comments
Open

Possible violation of the pthread_create interface? #4324

Dante-Broggi opened this issue Feb 21, 2023 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Dante-Broggi
Copy link
Contributor

So, I'm exploring the runtime codebase and make an assumption below about the behavior of restrict with respect to threads so I am probably wrong but I believe I found a violation of the pthread_create interface in the pony runtime:
In scheduler.c
The runtime calls ponyint_thread_create with a pointer to a scheduler's tid and a pointer the whole scheduler,

    if(!ponyint_thread_create(&scheduler[i].tid, run_thread, scheduler[i].cpu,
      &scheduler[i]))

Note that run_thread accesses sched->cpu(*sched).cpu(*(scheduler_t*) arg).cpu which is an access to *(scheduler_t*) arg before any synchronization.

static DECLARE_THREAD_FN(run_thread)
{
  scheduler_t* sched = (scheduler_t*) arg;
  this_scheduler = sched;
  ponyint_cpu_affinity(sched->cpu);

ponyint_thread_create calls pthread_create with those arguments,

  if(pthread_create(thread, attr_p, start, arg))

The POSIX docs say pthread_create is declared:

int pthread_create(pthread_t *restrict thread,
       const pthread_attr_t *restrict attr,
       void *(*start_routine)(void*), void *restrict arg);

In addition the POSIX docs for pthread_create do not specify whether or not the new thread begins executing before pthread_create itself returns, so I assume it does.

The behavior of restrict is complicated, and I am only referencing a draft of C11, but if the assumption above extends to the definition of restrict, the beginning of start_routine (on the new thread) occurs "During [the] execution of [the block associated with the restrict qualified pointer arg]" start_routine accesses the lvalue *(scheduler_t*) arg which has &(*(scheduler_t*) arg) based on arg, *(scheduler_t*) arg is modified by the write to *thread by pthread_create, but this is an access to the value of *(scheduler_t*) arg which is not based on arg and thus the behavior is undefined.
So, did I do a mistake in my reasoning, is my assumption wrong, or is this actually undefined?
I doubt anything but a sanitizer would behave contrary to expectations even so, but It would be wrong regardless.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 21, 2023
@Dante-Broggi
Copy link
Contributor Author

@jemc
Copy link
Member

jemc commented Feb 21, 2023

I think I was convinced over the duration of the sync call that this is indeed a spec violation (but I may still be under-informed on the issue, so I could be wrong). That is, I was convinced that some future version of clang could add an optimization that leverages the restrict qualifier in a way that breaks our usage of pthread_create.

The violation seems to be that our first arg and final arg to pthread_create are overlapping in memory, because the pony_thread_id_t struct reference we pass as the first arg is embedded within the scheduler_t reference which we pass as the final arg. Because they address an overlapping memory region, certain optimizations allowed by the restrict qualifier could cause a copy operation to clobber the view as seen by one or the other.

One way to avoid this would be to not embed the pony_thread_id_t - we could, for example, pool-allocate it instead. Or keep it in a separate struct array perhaps.

@SeanTAllen
Copy link
Member

I'm not convinced this is an issue. We should discuss together at some point @jemc. I agree it is a violation of the quoted posix spec but I'm not sure that matters, what matters is whether that spec is implemented by our target platforms.

@SeanTAllen
Copy link
Member

Joe and I discussed at sync today. We both agree this could be an issue.

I'm more worried that someone fixing this issue is likely to introduce bugs than clang and other compilers will start to follow this portion of the spec. I'm fine with someone fixing the issue herein so long as there is a test plan to make sure no bugs are introduced with the change to address the spec violation.

@SeanTAllen SeanTAllen added help wanted Extra attention is needed enhancement New feature or request and removed discuss during sync Should be discussed during an upcoming sync labels Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants