User Details
- User Since
- Jul 25 2016, 12:54 PM (348 w, 4 d)
Yesterday
Thu, Mar 30
I'm still not entirely a fan of the brittle test that tries to link in two different ways though, but I guess it can be tolerable if it's only this one test.
LGTM, thanks!
Overall LGTM, but I think it maybe would be nicer to split out the long->intptr_t changes into a separate patch.
Wed, Mar 29
Ping
Ping @phosek
Tue, Mar 28
Looks reasonable overall I think, nothing to object to from my point of view, just a few minor comments.. The patch is a bit unwieldy with the number of tests marked as unsupported, but I guess it's as good as it will be. If we're only touching tests that use specifically clang_cl_* to build/run, the mass editing probably is ok.
LGTM overall.
I tested this, and this does fix the repro from the linked issue.
Btw, for issues like these, it would be super useful if there was a CI configuration that builds libcxx/libunwind/libcxxabi from scratch in an environment where there's no preexisting unwinder or C++ library.
Thanks for the revert - I was also just looking into a build issue that seems to be caused by this commit.
Superseded by the improved version of D146894.
Mon, Mar 27
LGTM, thanks! This is indeed the neatest solution so far!
Sun, Mar 26
Sat, Mar 25
Fri, Mar 24
Reordered the checks to check for "clang-<major>" before plain "clang", as suggested in the linked discussion.
It seems like this change has broken building OpenMP with MSVC as a toplevel project from the llvm-project/llvm directory. To reproduce, make a build directory as a subdirectory to llvm-project/llvm, and configure it with cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;openmp". When I try to build with ninja, I get the following error:
ninja: error: 'projects/openmp/runtime/src/libomp.dll.lib', needed by 'lib/libomp.lib', missing and no known rule to make it
It seems like some of the subdirectory path prefixes end up applied inconsistently breaking the dependency chain here, making ninja refuse to proceed.
@fsb4000 Can you have a look at this, does it seem reasonable to you? (As mentioned in the patch description, I'm not a fan of the amount of ifdefs that this adds, but I didn't see a good way of getting away with fewer of them either.
Thu, Mar 23
Wed, Mar 22
FYI, most (all?) use of llvm::Optional has been migrated to std::optional these days.
Maybe adjust the commit message a little,
Tue, Mar 21
Mon, Mar 20
Rebased on latest git, to get a cleaner CI run.
Sun, Mar 19
Sat, Mar 18
Thu, Mar 16
Wed, Mar 15
Looks reasonable I guess - but I think it would be good to mention the env variables INCLUDE and LIB too, for alternative ways of finding the same things - even if it's not strictly the same as what this new section talks about.
Tue, Mar 14
Looks reasonable to me, but I don't feel entirely confident about what this does in the common MC layer changes.
Thanks for the patch - I've consiered doing something about this at some time. At the time, I considered filling areas with a more specific pattern, mapping to trapping instructions similar to int3, but since it requires multibyte patterns instead of filling with a single byte, I never got to doing anything about it.
Adding @aaron.ballman as fallback Clang reviewer here. While I did touch code in the vicinity of this area recently, I'm not familiar enough with the whole area to take on reviewing it at the moment.
Mon, Mar 13
I took a look at what's going wrong here; I diffed the cmake output from a cmake run before and after this change:
@@ -17,12 +17,12 @@ -- Performing Test LLVM_RUNTIMES_LINKING_WORKS -- Performing Test LLVM_RUNTIMES_LINKING_WORKS - Success -- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG --- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Failed +-- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Success -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG - Failed -- Using Release VC++ CRT: MD -- Performing Test SUPPORTS_BREPRO --- Performing Test SUPPORTS_BREPRO - Success +-- Performing Test SUPPORTS_BREPRO - Failed -- Looking for os_signpost_interval_begin -- Looking for os_signpost_interval_begin - not found -- Found Python3: /usr/bin/python3.10 (found version "3.10.6") found components: Interpreter @@ -33,8 +33,6 @@ -- Using libc++ testing configuration: /home/martin/code/llvm-project/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG - Failed --- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG --- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG - Failed -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA - Failed -- Performing Test CXX_SUPPORTS_FALIGNED_ALLOCATION_FLAG @@ -44,71 +42,65 @@ -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG - Failed -- Performing Test CXX_SUPPORTS_W4_FLAG --- Performing Test CXX_SUPPORTS_W4_FLAG - Success +-- Performing Test CXX_SUPPORTS_W4_FLAG - Failed -- Performing Test CXX_SUPPORTS_WEXTRA_FLAG
Fri, Mar 10
Thu, Mar 9
LGTM