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

[Hubert Tong via llvm-commits]

On Fri, Mar 15, 2019 at 6:56 PM Zachary Turner <zturner at google.com> wrote:

> Given that the header is arguably defective, imo this unfortunate hack is
> justifiable. Please leave a comment explaining it though
>
Will do.


> On Fri, Mar 15, 2019 at 3:54 PM Hubert Tong via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> 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>
>> ----------------
>> 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`.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D58801/new/
>>
>> https://reviews.llvm.org/D58801
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190316/64db8e40/attachment.html>