This is an archive of the discontinued LLVM Phabricator instance.

[Support] Implement is_local_impl with AIX mntctl
ClosedPublic

Authored by hubert.reinterpretcast on Feb 28 2019, 4:19 PM.

Details

Summary

On AIX, we can determine whether a filesystem is remote using mntctl.

If the information is not found, then claim that the file is remote (since that is the more restrictive case).

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2019, 4:19 PM
hubert.reinterpretcast marked an inline comment as done.Mar 1 2019, 12:21 PM
hubert.reinterpretcast added inline comments.
lib/Support/Unix/Path.inc
459

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.

hubert.reinterpretcast marked an inline comment as not done.Mar 1 2019, 12:32 PM

Ping. I am looking for some input on whether we want to pursue additional testing.

Ping. I am looking for some input on whether we want to pursue additional testing.

Yes, testing this is hard because we can have all possible combinations of the source and the build directory being remote / local.

I think creating a directory and checking that a contained file returns the same value as the directory should work. However I'm not sure how much value this adds. If it's reasonably easy to implement I think it makes sense, but personally I wouldn't see it as requirement for this patch

apaprocki added inline comments.
lib/Support/Unix/Path.inc
81

sys/statfs.h includes sys/types.h, which contains this typedef. Was there a reason why it was put here between the two headers?

hubert.reinterpretcast marked 2 inline comments as done.Mar 6 2019, 7:27 PM
hubert.reinterpretcast added inline comments.
lib/Support/Unix/Path.inc
81

Yes, the header is defective. It needs a typedef that is only conditionally defined.

apaprocki added inline comments.Mar 6 2019, 9:01 PM
lib/Support/Unix/Path.inc
81

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;
$
hubert.reinterpretcast marked 3 inline comments as done.Mar 6 2019, 9:38 PM
hubert.reinterpretcast added inline comments.
lib/Support/Unix/Path.inc
81

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
xingxue accepted this revision.Mar 15 2019, 8:51 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Mar 15 2019, 8:51 AM
krytarowski added inline comments.Mar 15 2019, 12:37 PM
lib/Support/Unix/Path.inc
81

This typedef looks suspicious, there shall be a way to workaround it.

hubert.reinterpretcast marked an inline comment as done.Mar 15 2019, 1:07 PM
hubert.reinterpretcast added inline comments.
lib/Support/Unix/Path.inc
81

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).

krytarowski added inline comments.Mar 15 2019, 1:19 PM
lib/Support/Unix/Path.inc
81

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.

lib/Support/Unix/Path.inc
81

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.

krytarowski added inline comments.Mar 15 2019, 1:46 PM
lib/Support/Unix/Path.inc
81

How about restricting _ALL_SOURCE to this file only?

#define _ALL_SOURCE // Needed for uint typedef
#include <sys/statfs.h>
lib/Support/Unix/Path.inc
81

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.

hubert.reinterpretcast marked 9 inline comments as done.

Address review comments, add test, and apply style changes

  • Add comment re: the uint typedef
  • Add modified version of the unit test from rL295768
  • Use CamelCase for variable names; improve comments for error cases
lib/Support/Unix/Path.inc
81

Comment added as requested on the review thread on the mailing list.

459

The test is now added.

ormris removed a subscriber: ormris.Mar 22 2019, 3:45 PM

@xingxue, thanks for the review. I've updated the patch; please let me know if you have concerns with the updates. I'll probably commit this later this week.

This revision was automatically updated to reflect the committed changes.
mstojanovic added inline comments.Apr 24 2019, 10:15 AM
llvm/trunk/unittests/Support/Path.cpp
1495 ↗(On Diff #192941)

This test fails if /tmp in on an nfs. I believe the original test was removed in rL297260 for the same reason.

hubert.reinterpretcast marked an inline comment as done.Apr 24 2019, 2:55 PM
hubert.reinterpretcast added inline comments.
llvm/trunk/unittests/Support/Path.cpp
1495 ↗(On Diff #192941)

This checks that a file and its containing directory are equally local or remote. If you are noticing a failure, then it seems the implementation of the query on your system may be reporting on the mountpoint instead of the mounted filesystem.

mstojanovic added inline comments.Apr 25 2019, 2:28 AM
llvm/trunk/unittests/Support/Path.cpp
1495 ↗(On Diff #192941)

The problem isn't in the actual test, it's in the cleanup process afterwards. The temp file isn't really deleted, only silly renamed into .nfsXXXXX and when the program tries to delete the file-system-test-YYYYY directory it outputs an error message: Directory not empty.
The simplest way to reproduce this is to hard code the TestDirectory path to a directory created on an nfs.

mstojanovic added inline comments.Apr 25 2019, 7:06 AM
llvm/trunk/unittests/Support/Path.cpp
1495 ↗(On Diff #192941)

Adding ::close(FD) at the end seems to fix it.

hubert.reinterpretcast marked 3 inline comments as done.Apr 25 2019, 7:33 AM
hubert.reinterpretcast added inline comments.
llvm/trunk/unittests/Support/Path.cpp
1495 ↗(On Diff #192941)

@mstojanovic, that seems to be the right solution. Thanks for diagnosing the problem. Would you be committing that directly? I certainly would not mind if you did.

mstojanovic added inline comments.Apr 25 2019, 9:20 AM
llvm/trunk/unittests/Support/Path.cpp
1495 ↗(On Diff #192941)

Ok I'll commit it directly.