Various minor changes

[tkchia]

• Fix an issue in ./configure test for dup3()
  • the compiler was warning — rightly — that a call to open() with O_CREAT did not specify file permissions
• Also, make ./configure add to $CPPFLAGS — rather than clobbering it — if the host platform does not have its own <stdatomic.h>
• Test if host defines a fdatasync() function; if not, fall back on using fsync()
• [metal] When entering an interrupt handler via int, int3, etc., besides clearing the IF flag, also remember to clear the TF and AC flags
• [metal] Try to simplify code for dealing with the BIOS data area

[ghaerr]

Hello @tkchia,

I am working on a patch to add fdatasync() to the ./configure checks.

Great, thanks. There seems to be conflicting evidence as to whether fdatasync exists at all, or whether it's just not declared in the library headers. Reference this article.

I am not sure what to do about the atomic_exchange() calls

I think it has something to do with HAVE_FORK in blink/atomic.c, as <stdatomic.h> is included if the former is defined. The same HAVE_FORK, if removed from config.h, also allows signal.c to be compiled without segfaulting.

Thank you!

[tkchia]

@ghaerr : does macOS have <stdatomic.h> and atomic_exchange()/atomic_exchange_explicit()?

[ghaerr]

@tkchia: grepping /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include shows no stdatomic.h nor atomic_exchange. man 3 atomic_exchange does show a BSD man page though.

There does seem to be some comments about deprecation of some OS atomic routines in favor of stdatomic.h, but this is in an libkern/OSAtomic.h header. I can post some of these headers if you'd like.

[tkchia]

There does seem to be some comments about deprecation of some OS atomic routines in favor of stdatomic.h, but this is in an libkern/OSAtomic.h header. I can post some of these headers if you'd like.

@ghaerr : thanks. I see that Apple has put up some of their various versions of libc headers on their Apple Open Source site. If what I gather is correct, older versions of macOS libc (before 997.x) have <libkern/OSAtomic.h>, while newer versions omit it.

Thank you!

[tkchia]

Hmm... ./configure has this:

if ! config stdatomic "checking for stdatomic.h... "; then
  CPPFLAGS="-isystem tool/stdatomic"
fi

So it looks like this part is not working well on macOS? :thinking:

[ghaerr]

@tkchia:

older versions of macOS libc (before 997.x) have <libkern/OSAtomic.h>, while newer versions omit it.

Both my macOS systems do have that header file, and libkern/OSAtomic.h includes libkern/OSAtomicDeprecated.h. However, that file does not contain an atomic_exchange definition.

So it looks like this part is not working well on macOS?

Well, there is no tool/stdatomic on macOS that I have been able to find either.

[tkchia]

So it looks like this part is not working well on macOS?

Well, there is no tool/stdatomic on macOS that I have been able to find either.

@ghaerr : @jart included a "polyfill" implementation of <stdatomic.h> inside the Blink tree. By right this should kick in if your host platform lacks its own <stdatomic.h>.

Thank you!

[ghaerr]

@tkchia:

Grepping the usr/include directory shows the following:

Work-iMac2:include greg$ gr stdatomic .
./libkern/OSAtomic.h: * OSAtomic interfaces in terms of the <stdatomic.h> primitives.
./libkern/OSAtomicDeprecated.h: * The C11 interfaces in <stdatomic.h> resp. C++11 interfaces in <atomic>
./libkern/OSAtomicDeprecated.h: * interfaces in terms of the <stdatomic.h> resp. <atomic> primitives.
./libkern/OSAtomicDeprecated.h:     "Use " #_r "() from <stdatomic.h> instead"
./libkern/OSAtomicDeprecated.h:     "Use " #_r "_explicit(memory_order_relaxed) from <stdatomic.h> instead"
./libkern/OSAtomicDeprecated.h: * C11 <stdatomic.h> resp. C++11 <atomic> primitives.
./libkern/OSAtomicDeprecated.h:#if !(__has_include(<stdatomic.h>) && __has_extension(c_atomic))
./libkern/OSAtomicDeprecated.h:#error Cannot use inlined OSAtomic without <stdatomic.h> and C11 atomics
./libkern/OSAtomicDeprecated.h:#include <stdatomic.h>
./module.modulemap:  module stdatomic {
./module.modulemap:   header "stdatomic.h" // note: supplied by the compiler. <--- NOTE THIS LINE
Work-iMac2:include greg$ pwd
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include

Is stdatomic.h supposed to be supplied by clang, perhaps?

[ghaerr]

@tkchia:

@jart included a "polyfill" implementation of inside the Blink tree. By right this should kick in if your host platform lacks its own .

I see. I'll check on more details on whether CPPFLAGS="-isystem tool/stdatomic" ${CPPFLAGS} works on macOS.

[tkchia]

Hello @ghaerr,

Is stdatomic.h supposed to be supplied by clang, perhaps?

Wait a minute — what does config.log (in the Blink build tree) say about the <stdatomic.h> check?

---

On my system, yes, <stdatomic.h> is part of the GCC header files, rather than GNU libc. So I presume that <stdatomic.h> on your system is indeed part of clang.

In either case, if ./configure fails to find <stdatomic.h>, then the Blink build system should fall back on tool/stdatomic/stdatomic.h.

Best to look through your build logs to find out what is going on.

Thank you!

[ghaerr]

Hello @tkhcia,

what does config.log (in the Blink build tree) say about the check?

When configured using ./configure --disable-all --enable-metal --enable-vfs, config.log shows the following (no error) for the tool/config/stdatomic.c check:

========================================================================
checking for stdatomic.h...
========================================================================

cc -fno-common -g -O2 -fno-omit-frame-pointer -Wno-strict-aliasing -D_FILE_OFFSET_BITS=64 -D_DARWIN_C_SOURCE -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_GNU_SOURCE -Werror -o o/tool/config/stdatomic tool/config/stdatomic.c -lz -lm
o/tool/config/stdatomic

Then, going ahead and adding the following line to tool/config/stdatomic.c also works, indicating that macOS does have <stdatomic.h>:

// checks for c11 atomics
#include <stdatomic.h>

atomic_int x;

int main(int argc, char *argv[]) {
  if (atomic_exchange_explicit(&x, 123, memory_order_relaxed) != 0) return 1;
  if (atomic_exchange_explicit(&x, 456, memory_order_relaxed) != 123) return 2;
  if (atomic_exchange(&x, 789) != 456) return 3; // <-- ADDED THIS LINE
  return 0;
}

I have not looked at much of the code that uses atomic_* functions, but configure indicates the following:

# c11 atomics are no-op'd when both fork and threads are disabled
# this causes code to break modern compiler strict aliasing rules
# TODO(jart) is it safe to quash warning w/o -fno-strict-aliasing
if [ -n "${DISABLE_THREADS}" ] && [ -n "${DISABLE_FORK}" ]; then
  EXTRA_CFLAGS=-Wno-strict-aliasing
  if config noop "checking for -Wno-strict-aliasing... "; then
    CFLAGS="${CFLAGS} -Wno-strict-aliasing"
  fi
fi

I note that in my config.h, HAVE_FORK is commented out, and there is no HAVE_THREADS at all.

When I change the following line in blink/atomics.h, the macOS atomics compilation problems go away, but I'm not sure this is the appropriate fix for this issue (perhaps there should be a HAVE_ATOMICS):

#ifndef BLINK_ATOMIC_H_
#define BLINK_ATOMIC_H_
#include "blink/builtin.h"
#include "blink/thread.h"
#if defined(HAVE_FORK) || defined(HAVE_THREADS) || defined(__APPLE__) // <-- Changed for macOS
#include <stdatomic.h>
#else

In summary, with your changes and the above change to blink/atomic.h, blink compiles with VFS enabled without atomic errors, but still crashes compiling blink/signal.c.

Thank you!

[ghaerr]

Hello @tkchia, hello @jart,

I have debugged the macOS clang crash and identified the following statement in blink/signal.c (line 267) which crashes the compiler:

void EnqueueSignal(struct Machine *m, int sig) {
  if (m && (1 <= sig && sig <= 64)) {
    //if ((m->signals |= 1ul << (sig - 1)) & ~m->sigmask) { // <-- leaving this statement in crashes clang!
    m->signals |= 1ul << (sig - 1); // <-- replaced the above line with these 2 works
    if (m->signals & ~m->sigmask) { // 2nd line replaced
      atomic_store_explicit(&m->attention, true, memory_order_release);
    }   
  }
}

Would you like to add this fix to this PR? That should fix that problem.

After fixing the above, I find one additional macOS compilation problem, which is that even when configured with VFS enabled and threads disabled using ./configure --disable-all --enable-metal --enable-vfs --disable-threads, we gets the following on macOS when compiling blink/vfs.c (DISABLE_THREADS is defined in config.h):

cc -fno-common -g -O2 -fno-omit-frame-pointer -Wno-strict-aliasing -U_FORTIFY_SOURCE -D_FILE_OFFSET_BITS=64 -D_DARWIN_C_SOURCE -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_GNU_SOURCE -iquote.  -c -o o//blink/vfs.o blink/vfs.c
blink/vfs.c:356:13: error: implicit declaration of function 'pthread_mutex_lock' is
      invalid in C99 [-Werror,-Wimplicit-function-declaration]
  unassert(!pthread_mutex_lock(&device->lock));
            ^
blink/vfs.c:359:17: error: implicit declaration of function 'pthread_mutex_unlock' is
      invalid in C99 [-Werror,-Wimplicit-function-declaration]
      unassert(!pthread_mutex_unlock(&device->lock));
                ^
blink/vfs.c:373:13: error: implicit declaration of function 'pthread_mutex_unlock' is
      invalid in C99 [-Werror,-Wimplicit-function-declaration]
  unassert(!pthread_mutex_unlock(&device->lock));
            ^
3 errors generated.
gmake: *** [build/rules.mk:14: o//blink/vfs.o] Error 1

I'm not sure exactly how this should be fixed.

Thank you!

[ghaerr]

Hello @tkchia,

Your most recent push allows macOS builds to get much further. Using ./configure --disable-all --enable-metal --enable-vfs --disable-threads we get the following errors:

cc -fno-common -g -O2 -fno-omit-frame-pointer -Wno-strict-aliasing -U_FORTIFY_SOURCE -D_FILE_OFFSET_BITS=64 -D_DARWIN_C_SOURCE -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_GNU_SOURCE -iquote.  -c -o o//blink/procfs.o blink/procfs.c
blink/procfs.c:556:16: error: implicit declaration of function 'atomic_exchange' is
      invalid in C99 [-Werror,-Wimplicit-function-declaration]
    opendata = atomic_exchange(&procinfo->opendir, NULL);
               ^
blink/procfs.c:556:14: warning: incompatible integer to pointer conversion assigning to
      'void *' from 'int' [-Wint-conversion]
    opendata = atomic_exchange(&procinfo->opendir, NULL);
             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
blink/procfs.c:558:16: error: implicit declaration of function 'atomic_exchange' is
      invalid in C99 [-Werror,-Wimplicit-function-declaration]
    opendata = atomic_exchange(&procinfo->openfile, NULL);
               ^
blink/procfs.c:558:14: warning: incompatible integer to pointer conversion assigning to
      'void *' from 'int' [-Wint-conversion]
    opendata = atomic_exchange(&procinfo->openfile, NULL);
             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings and 2 errors generated.
gmake: *** [build/rules.mk:14: o//blink/procfs.o] Error 1

Now that I understand it better, I think the problem is that the purpose of blink/atomic.h is twofold: it either includes <stdatomic.h> when required, or it supplies no-op versions of the same functions when not. Thus, IMO the real problem is that blink/atomic.h does not define atomic_exchange. It appears that perhaps no-one has tried compilation of blink/procfs.c with this option enabled, and it is the only file which uses atomic_exchange vs atomic_exchange_explicit.

Note that a no-op version of atomic_exchange_explicit is defined. I am not sure of the difference between the two.

Thank you!

[ghaerr]

@tkchia: I have confirmed that the changes thus far in this PR now allow successful compilation of blink on macOS using all default options, i.e. ./configure. Thus, the only remaining problem is that blink/procfs.c uses the atomic_exchange function for which there is not a no-op version defined for the case where one is not needed (e.g. !HAVE_FORK && !HAVE_THREADS).

Thank you!

[tkchia]

@ghaerr : thanks. It should hopefully be straightforward to add such a thing to "blink/atomic.h".

[tkchia]

@ghaerr : OK, now I am getting compiler errors relating to atomic_fetch_sub( ) and atomic_fetch_add( ). Let me see if I can fix these.

[ghaerr]

@tkchia:

I am getting compiler errors relating to atomic_fetch_sub( ) and atomic_fetch_add( )

Yes, I see now: it appears that that the VFS code is using non-"atomic_XXX_explicit" versions of the atomic functions, none of which are handled in blink/atomic.h for the case where atomic functions are effective no-ops.

[tkchia]

@ghaerr : still working on it...

./configure --disable-all --enable-metal --enable-vfs --disable-threads

make -j4

$ o//blink/blinkenlights -r -t o//test/metal/biosdisk.bin
error: vfs initialization failed

[ghaerr]

@tkchia:

still working on it... ./configure --disable-all --enable-metal --enable-vfs --disable-threads

Looking at blink/atomic.h, I can see that implementing a no-op version of atomic_exchange may be harder than I thought, as it should return the type specified in its first argument. VfsInit is also complicated. Since your existing PR fixes the macOS bugs and others that you originally intended, perhaps committing this PR now and leaving the atomic fixes for another PR would be a good idea?

I can use ./configure with your changes in this PR and keep moving forward...

[EDIT: It is also possible VFS is not well tested against various less-used configure options...]

Thank you!

[tkchia]

perhaps committing this PR now and leaving the atomic fixes for another PR would be a good idea?

@ghaerr : OK. Will do that. Thank you!