test/atomics.cpp compile error on Linux: 'wait' is ambiguous

[Dan Bonachea]

test/atomics.cpp fails to build on Linux with g++ 7.1.0 (also clang++), as shown below.

It appears the Linux standard libraries include a conflicting symbol named wait

M -MT x /home/pcp1/bonachea/UPC/bex/dbg/upcxx/test/atomics.cpp
/usr/local/pkg/gcc/7.1.0/bin/g++ -std=c++11 -D_GNU_SOURCE=1 -I/home/pcp1/bonachea/UPC/bex/dbg/upcxx/.nobs/art/9e4cda53be3870dd3137a03d49b46e69ee557271 -DUPCXX_BACKEND=gasnet1_seq -D_GNU_SOURCE=1 -DGASNET_SEQ -I/home/pcp1/bonachea/UPC/bex/dbg/upcxx/gasnet/include -I/home/pcp1/bonachea/UPC/bex/dbg/upcxx/gasnet/include/smp-conduit -O0 -g -Wall -g3 -Wall -Wpointer-arith -Wwrite-strings -Wmissing-format-attribute -Wno-unused -Wno-unused-parameter -Wno-address -c /home/pcp1/bonachea/UPC/bex/dbg/upcxx/test/atomics.cpp -o /home/pcp1/bonachea/UPC/bex/dbg/upcxx/.nobs/art/639337727b8e636c1dde98912580fe56bce96700.atomics.cpp.o
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/pkg/gcc/7.1.0/bin/g++ -std=c++11 -D_GNU_SOURCE=1 -I/home/pcp1/bonachea/UPC/bex/dbg/upcxx/.nobs/art/9e4cda53be3870dd3137a03d49b46e69ee557271 -DUPCXX_BACKEND=gasnet1_seq -D_GNU_SOURCE=1 -DGASNET_SEQ -I/home/pcp1/bonachea/UPC/bex/dbg/upcxx/gasnet/include -I/home/pcp1/bonachea/UPC/bex/dbg/upcxx/gasnet/include/smp-conduit -O0 -g -Wall -g3 -Wall -Wpointer-arith -Wwrite-strings -Wmissing-format-attribute -Wno-unused -Wno-unused-parameter -Wno-address -c /home/pcp1/bonachea/UPC/bex/dbg/upcxx/test/atomics.cpp -o /home/pcp1/bonachea/UPC/bex/dbg/upcxx/.nobs/art/639337727b8e636c1dde98912580fe56bce96700.atomics.cpp.o
/home/pcp1/bonachea/UPC/bex/dbg/upcxx/test/atomics.cpp: In function 'void test_fetch_add(bool)':
/home/pcp1/bonachea/UPC/bex/dbg/upcxx/test/atomics.cpp:31:9: error: reference to 'wait' is ambiguous
         wait(atomic_put(target_counter, (int64_t)0, memory_order_relaxed));
         ^~~~
In file included from /usr/include/stdlib.h:42:0,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/cstdlib:75,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/ext/string_conversions.h:41,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/bits/basic_string.h:6159,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/string:52,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/bits/locale_classes.h:40,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/bits/ios_base.h:41,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/ios:42,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/ostream:38,
                 from /usr/local/pkg/gcc/7.1.0/include/c++/7.1.0/iostream:39,
                 from /home/pcp1/bonachea/UPC/bex/dbg/upcxx/test/atomics.cpp:1:
/usr/include/bits/waitstatus.h:66:7: note: candidates are: union wait
 union wait
       ^~~~
In file included from /home/pcp1/bonachea/UPC/bex/dbg/upcxx/test/atomics.cpp:10:0:
/home/pcp1/bonachea/UPC/bex/dbg/upcxx/.nobs/art/9e4cda53be3870dd3137a03d49b46e69ee557271/upcxx/wait.hpp:9:8: note:                 template<class Kind, class ... T> decltype (f.result()) upcxx::wait(const upcxx::future1<Kind, T ...>&)
   auto wait(future1<Kind, T...> const &f)
        ^~~~
(message repeats)

I suspect this means we want using upcxx::wait; in upcxx/wait.hpp to keep users from running into this issue, but I'm uncertain what implications this might have.

[Dan Bonachea]

fix issue #22: wait ambiguity on Linux

→ <<cset 11e27f76d935>>

[Former user]

I think the current solution is considered bad practice. The user needs to disambiguate wait themselves by using upcxx::wait at each call-site or doing the using upcxx::wait. The current solution makes the following user code impossible:

#include <upcxx/upcxx.hpp>
#include <stdlib.h>
using ::wait;

void foo() {
  wait(...); // expecting ::wait from stdlib.h
}

[Dan Bonachea]

A few comments:

• The 'wait' that gets included by stdlib.h on Linux is actually a union, not a function - so there's no way to "call" it as your code suggests. I'm not even sure why the C++ typechecker decides it's ambiguous.
• There is a POSIX wait() function defined in <sys/wait.h>, but when I add that include to your example program, it does compile and uses the system call:

#include <upcxx/upcxx.hpp>
#include <stdlib.h>
#include <sys/wait.h>
using ::wait;

int main()  {

  wait(0); // uses ::wait from sys/wait
  return 0;
}

• Even if this did not work, the system call in question is used to put an entire process to sleep while waiting for a forked child process. This is at best a non-portable behavior for an HPC app and at worst rather a dangerous thing to do when using some GASNet conduits, both because it makes the process inattentive and many HPC drivers do not deal well with fork(). So a UPC++ program trying to use this particular system call is probably already doing something wrong.

So although it's admittedly ugly I still don't see a practical downside to the current solution.

On the other hand, leaving it unfixed means forcing every UPCXX program to insert the qualifier for this one particular function (an error which only shows up on Linux), and seems likely to arise often enough to be a usability nuisance in practice.

If we're not happy with enforcing upcxx::wait() as the default wait function for modules that #include <upcxx/*>, then perhaps we should consider changing wait() to be a method on future, to prevent the ambiguity with the POSIX function of the same name. That's also less typing overall, and arguably cleaner OOP design.

[Steven Hofmeyr]

I agree with Dan: we don't want the user to have to qualify wait with upcxx::, when they don't have to qualify anything else. It's particularly confusing because stdlib.h is very often used, and most of the time, users are not including it for stdlib's wait; they may not even be aware that there is a stdlib wait. Another suggestion is to change the name of the function, although to what, I don't have any ideas.

[Former user]

using directives in headers is a form of "global namespace pollution", which is generally considered a bad thing. Clang has the option -Wusing-directive-in-header (included in the -Wheader-hygiene group) which asserts against exactly our solution.

The user scenario we're trying to support is also considered bad practice. using namespace upcxx is a "using directive" which, as I'm just learning, are there for backwards compatibility but discouraged in new code. Programmers are encouraged to either namespace-qualify each use-site, or put a "using declaration" (using upcxx::wait) in a delimited scope (source file or function scope). The google coding standard explicitly bans all use of using namespace ...; Our programming examples should be updated to reflect current best practice: no more using namespace upcxx. Instead, fully qualify the things you use rarely (like upcxx::init) and add "using declarations" for the rest:

#include <upcxx/upcxx.hpp>
using upcxx::wait;

int main() {
 upcxx::init();
 wait; wait; wait;
 upcxx::finalize();
}

[Dan Bonachea]

Is there a downside to changing wait() to be a method on upcxx::future?

That seems to make more sense from an OOP interface design perspective anyhow, and unless I'm mistaken it makes all the namespace issues disappear.

If we're worried about any "old" code that might exist, we can either leave upcxx::wait() as a deprecated function, or possibly just specify upcxx::future::wait() as syntactic sugar for upcxx::wait(*this).

[Former user]

The downside to a class method for wait() is that futures become tied to a specific progress engine (upcxx user-level). The fact that we phrase wait() as the progress engine waiting on the future as opposed to the future waiting on the engine feels like it allows futures to arbitrate asynchrony between concurrent engines more naturally. I already find that useful when managing events between upcxx's internal-progress and user-progress. But of course I never wait(), so binding just this method to a specific engine shouldn't bother other experts too much. This is purely conceptual, technically the distinction between a.f(b), b.f(a), and f(a,b) can be pretty thin, but I think its the best use of OOP methods when the state mutated by a method is restricted to the this object. In a.f(b), ideally at most a is modified and b and all other global state is readonly. The stl containers mostly behave this way: an operation that mutates two datastructues is usually a free function (counterpoint: C++ copy/move ctors do not work this way). In the case of wait(f) the other object is the entire runtime-state being passed implicitly. What we have now is like runtime.wait(future) which I prefer to future.wait(runtime) (but we could make that runtime argument actually be the progress callback to spin on and default it to upcxx::progress).

So technically, no downsides. My preference is based on personal taste. Since upcxx is a new library, I don't think we should be worrying about changing the aesthetics of our API to deal with deprecated coding style.

[Former user]

I'm currently implementing future.wait(), but would also like to propose an alternative. I think "wait" is a bad name. To me "waiting" implies that the current thread does nothing until some condition is met. But in our case the user-progress engine is running which could be executing lots of user code (from rpcs, lpcs, or rput/get completions) in the current thread. I feel like an alternate name, like T upcxx::progress_until(future<T>), having the same semantics as wait, would be less surprising. "progress_until" would no longer clash with the linux usage of "wait".

[Former user]

Fixed issue 22: https://bitbucket.org/berkeleylab/upcxx/issues/22/test-atomicscpp-compile-error-on-linux

Made wait() a method of future. Updated tests accordingly.

→ <<cset cc27dba4112a>>

[Dan Bonachea]

Re: naming

I agree the name progress_until() is more descriptive of what's happening in a future.wait(), but I think MPI users are also used to the idea that MPI_Wait() does not mean "idle" - it means run the MPI progress engine until the MPI_Request is satisfied. Our progress engine is notably different in that it frequently executes callbacks into user code, but MPI also has this property to a lesser extent (eg user-provided reduction operators).

It's also worth noting that future.wait is only one of (eventually) many upcxx:: functions that invoke user progress, so unless you're suggesting we insert the word "progress" into the name of every function name that does, I think this would set a confusing precedent.