[PATCH] D58801: [Support] Implement is_local_impl with AIX mntctl

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: lib/Support/Unix/Path.inc:81
+#include <sys/statfs.h>
+typedef uint_t uint;
+#include <sys/vmount.h>
----------------
hubert.reinterpretcast wrote:
> krytarowski wrote:
> > hubert.reinterpretcast wrote:
> > > krytarowski wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > krytarowski wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > apaprocki wrote:
> > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > apaprocki wrote:
> > > > > > > > > > `sys/statfs.h` includes `sys/types.h`, which contains this `typedef`. Was there a reason why it was put here between the two headers?
> > > > > > > > > Yes, the header is defective. It needs a typedef that is only conditionally defined.
> > > > > > > > I guess I am not seeing that.. is it OS version dependent?
> > > > > > > > 
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > $ cat test.c
> > > > > > > > #include <sys/statfs.h>
> > > > > > > > #include <sys/vmount.h>
> > > > > > > > $ xlc -E -c test.c | grep "typedef.*uint;"
> > > > > > > > typedef         uint_t          uint;
> > > > > > > > $
> > > > > > > > ```
> > > > > > > Try with `-D_XOPEN_SOURCE=700` or anything that suppresses `_ALL_SOURCE`:
> > > > > > > ```
> > > > > > > > xlclang -D_XOPEN_SOURCE=700 test.c -c
> > > > > > > In file included from test.c:2:
> > > > > > > /usr/include/sys/vmount.h:195:2: error: unknown type name 'uint'; did you mean 'int'?
> > > > > > >         uint    vmt_revision;   /* I revision level, currently 1        */
> > > > > > >         ^
> > > > > > > /usr/include/sys/vmount.h:196:2: error: unknown type name 'uint'; did you mean 'int'?
> > > > > > >         uint    vmt_length;     /* I total length of structure and data */
> > > > > > >         ^
> > > > > > > /usr/include/sys/vmount.h:203:2: error: unknown type name 'uint'; did you mean 'int'?
> > > > > > >         uint    vmt_time;       /* O time of mount                      */
> > > > > > >         ^
> > > > > > > /usr/include/sys/vmount.h:204:2: error: unknown type name 'uint'; did you mean 'int'?
> > > > > > >         uint    vmt_timepad;    /* O (in future, time is 2 longs)       */
> > > > > > >         ^
> > > > > > > 4 errors generated.
> > > > > > > Error while processing test.c.
> > > > > > > Return:  0x01:1
> > > > > > > ```
> > > > > > This typedef looks suspicious, there shall be a way to workaround it.
> > > > > What is your proposed solution to otherwise provide a type name required by the system header that might not be defined? Note that this code is guarded under `defined(_AIX)`, and it has been demonstrated that this typedef might be defined anyway by the system headers on that platform to be an alias for `uint_t` (which is, in turn, an alias for `unsigned int`).
> > > > I don't have access right now to AIX to check it myself, but on UNIX it's typically needed to include `<sys/types.h>` and/or specify `-D` flags like `-D_POSIX_SOURCE`.
> > > Yes, and `<sys/types.h>` needs `-D_ALL_SOURCE` before `uint` is provided, and `_ALL_SOURCE` on AIX produces a lot of stray macros. I'd like to avoid needing `_ALL_SOURCE` for the build if at all possible.
> > How about restricting `_ALL_SOURCE` to this file only?
> > 
> > ```
> > #define _ALL_SOURCE // Needed for uint typedef
> > #include <sys/statfs.h>
> > ```
> That would not work if `sys/types.h` is included by an earlier header, and defining `_ALL_SOURCE` earlier in `Path.cpp` would expose the LLVM headers included by that file to the stray macros that come with `_ALL_SOURCE`.
Comment added as requested on the review thread on the mailing list.


================
Comment at: lib/Support/Unix/Path.inc:446
 
 std::error_code is_local(const Twine &Path, bool &Result) {
   struct STATVFS Vfs;
----------------
hubert.reinterpretcast wrote:
> There's no testing for this. The previous attempt at having testing for this interface was removed in http://llvm.org/viewvc/llvm-project?view=revision&revision=297260. It seems that a reasonable expectation is that a newly created file and its parent directory would produce the same answer when queried. I am not sure if there is a desire to pursue adding such a test.
The test is now added.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58801/new/

https://reviews.llvm.org/D58801