jart / blink

tiniest x86-64-linux emulator
ISC License
6.9k stars 217 forks source link

remove threads.h from procfs.c., fixes darwin build #96

Closed aycanirican closed 1 year ago

aycanirican commented 1 year ago

Looks like vfs is WIP but let's don't break darwin build.

tkchia commented 1 year ago

Hello @aycanirican,

Unfortunately the build now fails on platforms that do have <threads.h>, when the VFS feature is enabled.

Instead of removing the #include <threads.h> entirely, I would suggest including the file only when VFS is enabled, i.e.

#ifndef DISABLE_VFS
#include <threads.h>
...
#endif

for the time being.

@jart, @trungnt2910 : I suppose a more long-term solution would be to get the VFS code to build on platforms that lack C11 threads support. What are your thoughts on this?

Thank you!

trungnt2910 commented 1 year ago

Currently the only thing that requires <threads.h> is this single thread_local variable:

  static thread_local char buf[sizeof(struct dirent) + VFS_NAME_MAX];
  struct dirent *de = (struct dirent *)buf;

I suppose this could be attached to the ProcfsOpenDir cookie instead? The original readdir function does not have any thread-safety guarantee anyway.

tkchia commented 1 year ago

Hello @trungnt2910,

Currently the only thing that requires <threads.h> is this single thread_local variable:

OK... if so, then we should be able to simply use a different keyword instead: either _Thread_local (C11), or __thread (GNU, pre-C11). Then there is no need to #include <threads.h>.

I suppose this could be attached to the ProcfsOpenDir cookie instead?

Well, I am not very familiar with the VFS code, so I guess I will defer to you on this question.

Thank you!

trungnt2910 commented 1 year ago

I think using _Thread_local is the right way to solve this problem as this is also what the rest of the blink codebase does.

aycanirican commented 1 year ago

@trungnt2910 @tkchia I agree that this needs to be fixed for VFS enabled builds. From what I understand from comments, I learned that we can use STDC_NO_THREADS pragma to detect if threads extension is present in the underlying implementation hence wrapped the threads.h with that. Also fixed storage-class specifier for thread local vars as you have mentioned.

Thank you.

jart commented 1 year ago

Thank you for the sending this to us @aycanirican. I hit approve too soon before seeing @tkchia's feedback.

@tkchia What platform requires threads.h? I've never heard of that header. Ohh. It looks like the C11 threads API. If we're using C11 threads, then I think it'd be better to just use POSIX threads. We require POSIX. Plus I'm reasonably certain that, even if we cared about MSVC, Microsoft doesn't implement either the C or C++ threads APIs.

jart commented 1 year ago

@tkchia I played around locally. I reproduced the build error. I'm reasonably certain threads.h isn't needed at all. So I think your concern has been addressed. I'll proceed with the merge.