- User Since
- Aug 21 2015, 4:29 PM (212 w, 6 d)
Fri, Sep 13
@thakis I gave this patch a try. It seems to work apart from the i386 tests that will not work on macOS Catalina. We can deal with that later though.
@thakis Sorry for the delay on this. I should have some time this afternoon to try this out.
@vsk Thanks for the review.
@vsk Thanks for the review.
Thu, Sep 12
Pass -w when calling try_compile_only(...) to make architecture detection more robust.
Thu, Sep 5
Aug 12 2019
Aug 8 2019
@vitalybuka Thanks for the review!
Aug 1 2019
Jul 31 2019
I think the way forward is for us to just make sure we have the full xcode available when building our clang package. But we need to figure out how to coax our infrastructure into making that happen..
Jul 30 2019
Jul 29 2019
@porglezomp Just to add a little more context here killing processes in lit has been a source of trouble for me and is part of why I never landed https://reviews.llvm.org/D47210 . We don't have a mechanism for tracking all the processes spawned during tests and when I implemented the --timeout flag I had to implement a hack where we try walk the process tree to kill all the processes spawned by a shell test.
Multiprocessing seems to complicate things because the parent process will just re-spawn a worker process if it's killed.
@porglezomp I think killing the process group is the intention. Shell tests can spawn many child processes and we want to make sure they get killed when abort_now() gets called. If you just kill the worker process you won't kill the child processes spawned by shell tests. This could cause zombie processes to be left lying around. For this reason I don't think we can land this patch.
Jul 24 2019
LGTM but I don't own this code so please allow time for the owners to speak up in case they object.
Jul 23 2019
@davide While I agree with sentiment of fixing psutil, the use of psutil was never meant to be permanent. As the owner of this code I don't mind making accommodations for other platforms provided we keep implementation details well contained as this patch now does.
Jul 18 2019
@daltenty Other than the minor nit, LGTM.
Jul 17 2019
Jul 11 2019
Jul 9 2019
Jul 5 2019
@AlexeySachkov LGTM provided the lit tests continue to pass.
@daltenty The "This removes our dependency on psutil." text sounds broader than it actually is. Looking at the implementation the removal of the dependency is only for AIX. All other platforms still depends on the psutil module. I think the commit message should be made clearer on this point.
Jun 27 2019
Jun 25 2019
Jun 24 2019
@vitalybuka Sorry. I didn't see this before.
Jun 21 2019
@hans @thakis Commenting out find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator) was enough for me to reproduce the issue. There's a bug in some existing CMake code I wrote. The code for UBSan test code assumes that if COMPILER_RT_ENABLE_IOS is true that the toolchain can build for iOS and the iOS simulator. This isn't true if one or more of the SDKs are missing. I've written a fix for this and I'll submit a patch for review soon.
But somehow it was still able to produce an i386 version of libclang_rt.asan_iossim_dynamic.dylib before your change.
Jun 20 2019
Jun 17 2019
@vitalybuka Sorry I forgot about this. I tried to make the test to work in a POSIX use case and I can't make it work. The problem is that when I use %M with stack_trace_format the module name gets stripped (i.e. an absolute path to the binary just becomes the file name of the binary) due to the call to StripModuleName(). This results in us not being able to symbolicate the report from the python script because we don't have the absolute path to the binaries we need to access to perform symbolication.
@juliehockett Sorry I completely dropped the ball here and forgot to land this. I've rebased and committed the change. @thakis I noticed I had a merge conflict with your change. Hopefully this hasn't broken anything for you.
Jun 11 2019
Apr 28 2019
@juliehockett @phosek This is a new version of https://reviews.llvm.org/D58578 . Would you be able to test it in your set up? I'd prefer to have the confidence of knowing it doesn't break your set up before landing this.
Apr 27 2019
Sure -- to clarify, it's not a test failure, it's a build one. The error was triggered during the CMake configuration step for host runtimes, where we have OSX supported arches: x86_64 only. test/tsan/CMakeLists.txt:78 now calls into the new function on the arm64 case, which down the line triggers a CMake assert that arm64 is in the list of supported arches when it isn't.
Seems like prior to this change, the lit tests were being configured without doing any checks on that list of supported arches. I'm not as familiar with how this code is set up, so I can't really say if it's a problem with this patch or if this patch is exposing another bug. Thanks for taking a look!
To clarify, this is failing when building runtimes for Darwin so shouldn't be Fuchsia specific, but it could be affected by some of the Clang defaults we set in our CMake build. We use lld as the default linker for Fuchsia but not for Darwin where use the system linker.
Apr 26 2019
Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.