Page MenuHomePhabricator

Fix DynamicLibraryTest.cpp on FreeBSD
ClosedPublic

Authored by dim on May 14 2017, 7:01 AM.

Details

Summary

After rL301562, on FreeBSD the following unittest failures are being reported:

******************** TEST 'LLVM-Unit :: Support/DynamicLibrary/DynamicLibraryTests/DynamicLibrary.Shutdown' FAILED ********************
Note: Google Test filter = DynamicLibrary.Shutdown
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DynamicLibrary
[ RUN      ] DynamicLibrary.Shutdown
/share/dim/src/llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:107: Failure
Value of: DL.isValid()
  Actual: false
Expected: true
/share/dim/src/llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:108: Failure
Value of: Err.empty()
  Actual: false
Expected: true
/share/dim/src/llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:112: Failure
Value of: SS != nullptr
  Actual: false
Expected: true

******************** TEST 'LLVM-Unit :: Support/DynamicLibrary/DynamicLibraryTests/DynamicLibrary.Overload' FAILED ********************
Note: Google Test filter = DynamicLibrary.Overload
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DynamicLibrary
[ RUN      ] DynamicLibrary.Overload
/share/dim/src/llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:65: Failure
Value of: DL.isValid()
  Actual: false
Expected: true
/share/dim/src/llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:66: Failure
Value of: Err.empty()
  Actual: false
Expected: true
/share/dim/src/llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:69: Failure
Value of: GS != nullptr && GS != &TestA
  Actual: false
Expected: true

This is because the test uses getMainExecutable("DynamicLibraryTests", Ptr);, and since the path does not contain any slashes, retrieving the main executable will not work.

I chose to reimplement getMainExecutable() for FreeBSD using sysctl(3), which is more reliable than fiddling with relative or absolute paths.

However, this approach might not fix any failures for other OSes. So I also added retrieval of the original argv[] from the GoogleTest framework, to use as a fallback.

I also thought that the final fallback name might be "./DynamicLibraryTests", but that will not work on Windows. Is this test run on Windows at all?

Diff Detail

Repository
rL LLVM

Event Timeline

dim created this revision.May 14 2017, 7:01 AM
dim edited the summary of this revision. (Show Details)May 14 2017, 7:02 AM
krytarowski added inline comments.May 14 2017, 7:13 AM
lib/Support/Unix/Path.inc
80 ↗(On Diff #98925)

|| defined ( __NetBSD__)

114 ↗(On Diff #98925)

drop defined (__NetBSD__)

169 ↗(On Diff #98925)

drop NetBSD ||

185 ↗(On Diff #98925)

|| defined(__NetBSD__)

187 ↗(On Diff #98925)
#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 mib[0] = CTL_KERN;
  mib[1] = KERN_PROC;
  mib[2] = KERN_PROC_PATHNAME;
  mib[3] = -1;
#else
 mib[0] = CTL_KERN;
  mib[1] = KERN_PROC_ARGS;
  mib[2] = -1;
  mib[3] = KERN_PROC_PATHNAME;
#endif
195 ↗(On Diff #98925)

drop defined (__NetBSD__)

dim added inline comments.May 14 2017, 7:16 AM
lib/Support/Unix/Path.inc
187 ↗(On Diff #98925)

Ah, I wasn't aware that NetBSD already supported KERN_PROC_PATHNAME. I downloaded NetBSD 7.1, and couldn't find the define in sys/sysctl.h, but I see it has been added recently.

krytarowski added inline comments.May 14 2017, 7:19 AM
lib/Support/Unix/Path.inc
187 ↗(On Diff #98925)

It was added after the -7 branch a year ago or so. In my opinion no point to support here in upstream sources fallback, better to handle it downstream. By the time of 5.0.0 release we might have 8.0 out (and almost surely branched).

krytarowski added inline comments.May 14 2017, 7:20 AM
lib/Support/Unix/Path.inc
185 ↗(On Diff #98925)
(defined (NetBSD) && defined(KERN_PROC_PATHNAME))
187 ↗(On Diff #98925)

However the fallback isn't that difficult, I will propose a code for it.

dim updated this revision to Diff 98926.May 14 2017, 8:04 AM
  • Move sys/sysctl.h include to a separate block, specifically for (Free|Net)BSD
  • Remove NetBSD from several #if statements
  • Use correct mib for NetBSD to retrieve process pathname
krytarowski accepted this revision.May 14 2017, 8:12 AM
This revision is now accepted and ready to land.May 14 2017, 8:12 AM
This revision was automatically updated to reflect the committed changes.