Page MenuHomePhabricator

phosek (Petr Hosek)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 4 2015, 5:44 PM (289 w, 4 d)

Recent Activity

Fri, Sep 18

phosek added a comment to D87928: Provide -fsource-dir flag in Clang.

This change is trying to address the issues raised in D83154. There are still some open questions:

  • Is -fsource-dir the best name for this flag?
  • I'm not sure if make_relative should be applied to all source paths, or only paths that start with SourceDir which would exclude system paths outside of the source directory (e.g. it's probably undesirable to relativize paths to /usr/include)?
  • If we decide to exclude source paths outside of the source directory, should we support -fsource-dir to be specified more then once to handle multiple source directories?
Fri, Sep 18, 12:23 PM · Restricted Project, Restricted Project
phosek added a comment to D83154: clang: Add -fcoverage-prefix-map.
In D83154#2277539, @rnk wrote:

Yeah, my goal is for the build system to be able to avoid having to embed PWD into the compiler flags. For example, I believe it is a goal of the LLVM gn build system for the build tree to be relocatable. If the compiler command lines need to contain absolute paths to make the paths in the coverage relative, we won't be able to achieve that goal.

Based on all the comments that have gone before, it sounds like we want a -fprofile-compilation-dir= flag. Once we have that, would this logic work?

  • if profile compilation dir set, absolutize with that as the root
  • if no profile compilation dir, absolutize with CWD
  • apply profile prefix map

Should -no-canonical-prefixes also be involved here? Unclear.

I originally considered -fprofile-compilation-dir= and perhaps -ffile-compilation-dir= to also handle cases like __FILE__ macro (-ffile-prefix-map and -fmacro-prefix-map suffer from the same issue as -fprofile-prefix-map), but I'm not sure if specifying compilation dir is the right approach. It makes sense for debug info because -fdebug-compilation-dir is used directly as DW_AT_comp_dir and also because it's a convention for source file paths in debug info to be relative to build directory, but it's not the case for paths in coverage mapping or __FILE__.

So what I'm considering now and started prototyping is a -fsource-dirflag (or maybe -fsource-root-dir, I'm open to suggestions for the name) where the semantics would be that if this flag is set, all source paths will be made relative to this directory. There's an open question whether this should also apply to sources outside of that directory, for example system header paths.

The example use case, assuming source is in /path/to/source and build is in /path/to/build, would be: clang -fdebug-compilation-dir . -fsource-dir ../source ../source/dir/file.c. In this case __FILE__ would expand to dir/file.c and that path would be also stored in coverage mapping.

Fri, Sep 18, 12:17 PM · Restricted Project
phosek added reviewers for D87928: Provide -fsource-dir flag in Clang: keith, rnk, vsk.
Fri, Sep 18, 12:17 PM · Restricted Project, Restricted Project
phosek requested review of D87928: Provide -fsource-dir flag in Clang.
Fri, Sep 18, 12:17 PM · Restricted Project, Restricted Project

Wed, Sep 16

phosek added a comment to D87732: [Support] Provide sys::path::guess_style.

I'm not convinced this is really correct. After all, a path with mixed separators (e.g. '/my/path\to\foo') could be a Windows path rooted at the current drive. This mixed style can sometimes happen when things get concatenated together, so I don't think it's compeletely unreasonable.

I'm also concerned that if we guess wrong, the behaviour will end up breaking. For example, if the path were a Windows path "/my/path\../to/foo", I'd expect a remove_dots call (without worrying about slash normalisation) to result in "/my/to/foo", but if the code is using Posix style, will it actually result in the path being left unchanged?

I can think of two competing alternatives:

  1. add an option to remove_dots to not do the separator canonicalization. I'm not sure whether this can be done unambiguously however - which separator should be removed when a directory is removed due to dots? The one before the removed parts? After them?
  2. change all '\' to '/' unconditionally. However, this might break Linux paths with '\' in.
Wed, Sep 16, 5:02 PM · Restricted Project
phosek added a reverting change for rGc57df3dc09e8: [lsan] Share platform allocator settings between ASan and LSan: rGe3fe203ec7f7: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Wed, Sep 16, 1:49 PM
phosek committed rGe3fe203ec7f7: Revert "[lsan] Share platform allocator settings between ASan and LSan" (authored by phosek).
Revert "[lsan] Share platform allocator settings between ASan and LSan"
Wed, Sep 16, 1:49 PM
phosek added a reverting change for D85930: [lsan] Share platform allocator settings between ASan and LSan: rGe3fe203ec7f7: Revert "[lsan] Share platform allocator settings between ASan and LSan".
Wed, Sep 16, 1:49 PM · Restricted Project
phosek added a comment to D83154: clang: Add -fcoverage-prefix-map.
In D83154#2277539, @rnk wrote:

Yeah, my goal is for the build system to be able to avoid having to embed PWD into the compiler flags. For example, I believe it is a goal of the LLVM gn build system for the build tree to be relocatable. If the compiler command lines need to contain absolute paths to make the paths in the coverage relative, we won't be able to achieve that goal.

Based on all the comments that have gone before, it sounds like we want a -fprofile-compilation-dir= flag. Once we have that, would this logic work?

  • if profile compilation dir set, absolutize with that as the root
  • if no profile compilation dir, absolutize with CWD
  • apply profile prefix map

Should -no-canonical-prefixes also be involved here? Unclear.

Wed, Sep 16, 1:40 PM · Restricted Project
phosek committed rGc57df3dc09e8: [lsan] Share platform allocator settings between ASan and LSan (authored by phosek).
[lsan] Share platform allocator settings between ASan and LSan
Wed, Sep 16, 1:31 PM
phosek closed D85930: [lsan] Share platform allocator settings between ASan and LSan.
Wed, Sep 16, 1:31 PM · Restricted Project

Tue, Sep 15

phosek updated the diff for D87732: [Support] Provide sys::path::guess_style.
Tue, Sep 15, 11:47 PM · Restricted Project
phosek added a comment to D87732: [Support] Provide sys::path::guess_style.

Hmm - probably simpler to justify this patch by including refactoring at least one (possibly more) existing uses you mentioned, to use this API - then I probably don't really need to understand what it's doing, just that it's generalizing some existing functionality.

Tue, Sep 15, 11:46 PM · Restricted Project
phosek updated the diff for D87732: [Support] Provide sys::path::guess_style.
Tue, Sep 15, 11:33 PM · Restricted Project
phosek updated the diff for D87732: [Support] Provide sys::path::guess_style.
Tue, Sep 15, 11:31 PM · Restricted Project
phosek reopened D87657: [DebugInfo] Remove dots from getFilenameByIndex return value.
Tue, Sep 15, 5:11 PM · Restricted Project
phosek added a comment to D87732: [Support] Provide sys::path::guess_style.

I had to revert D87657 because it broke several tests: the normalization resulted in slashes being converted to native format, but that's incorrect. I'd like to use this function to instead determine the style from the path inside the debug info.

Tue, Sep 15, 5:10 PM · Restricted Project
phosek requested review of D87732: [Support] Provide sys::path::guess_style.
Tue, Sep 15, 5:08 PM · Restricted Project
phosek added inline comments to D83154: clang: Add -fcoverage-prefix-map.
Tue, Sep 15, 11:42 AM · Restricted Project
phosek added a reverting change for rG042c23506869: [DebugInfo] Remove dots from getFilenameByIndex return value: rG9c73e5551043: Revert "[DebugInfo] Remove dots from getFilenameByIndex return value".
Tue, Sep 15, 10:08 AM
phosek committed rG9c73e5551043: Revert "[DebugInfo] Remove dots from getFilenameByIndex return value" (authored by phosek).
Revert "[DebugInfo] Remove dots from getFilenameByIndex return value"
Tue, Sep 15, 10:08 AM
phosek added a reverting change for D87657: [DebugInfo] Remove dots from getFilenameByIndex return value: rG9c73e5551043: Revert "[DebugInfo] Remove dots from getFilenameByIndex return value".
Tue, Sep 15, 10:08 AM · Restricted Project
phosek updated the diff for D87656: [llvm-dwarfdump] --show-sources option to show all sources.
Tue, Sep 15, 2:04 AM · Restricted Project
phosek committed rG58938b544b72: [NFC][DebugInfo] Use consistent regex group spelling (authored by phosek).
[NFC][DebugInfo] Use consistent regex group spelling
Tue, Sep 15, 1:50 AM
phosek committed rGc1f2fb5184ca: [DebugInfo] Support both forward and backward slashes in tests (authored by phosek).
[DebugInfo] Support both forward and backward slashes in tests
Tue, Sep 15, 1:00 AM

Mon, Sep 14

phosek committed rG042c23506869: [DebugInfo] Remove dots from getFilenameByIndex return value (authored by phosek).
[DebugInfo] Remove dots from getFilenameByIndex return value
Mon, Sep 14, 8:31 PM
phosek closed D87657: [DebugInfo] Remove dots from getFilenameByIndex return value.
Mon, Sep 14, 8:31 PM · Restricted Project
phosek added a comment to D87657: [DebugInfo] Remove dots from getFilenameByIndex return value.

So a trivial example is:

#include <string>
int main() {}

When compiled as:

clang++ test.cc -g3 -S -o test.S

I see paths like these in the output:

"/usr/local/google/home/phosek/fuchsia" "prebuilt/third_party/clang/linux-x64/bin/../include/c++/v1/cstddef"

We could go over driver code and make sure we always invoke sys::path::remove_dots inside methods like AddIncludePaths or addPathIfExists (see for example https://github.com/llvm/llvm-project/blob/b3afad046301d8bb1f4471aceaad704b87de3a69/clang/lib/Driver/ToolChains/Gnu.cpp#L2902 which where the .. in libc++ header path comes from), but maybe it'd be better to do the normalization uniformly when emitting debuginfo metadata?

Mon, Sep 14, 7:14 PM · Restricted Project
phosek added a comment to D87657: [DebugInfo] Remove dots from getFilenameByIndex return value.

Got any particular examples of this from LLVM's DWARF output? I suspect this sort of thing might come from/lead to path non-canonicalization which would make the line table bigger than it needs to be, so might be worth fixing there too.

Mon, Sep 14, 6:11 PM · Restricted Project
phosek requested review of D87657: [DebugInfo] Remove dots from getFilenameByIndex return value.
Mon, Sep 14, 5:51 PM · Restricted Project
phosek added a comment to D87656: [llvm-dwarfdump] --show-sources option to show all sources.

While this information can be extracted out of the existing llvm-dwarfdump output, it requires additional post-processing. This became so common in our project that we have implemented a custom Go-based tool for that purpose, but that has other downsides such as the lack of DWARF5 support. I think that supporting this option directly in llvm-dwarfdump might be generally useful and it doesn't add a lot of complexity. I'm open to suggestions for how to improve the output.

Mon, Sep 14, 5:40 PM · Restricted Project
phosek requested review of D87656: [llvm-dwarfdump] --show-sources option to show all sources.
Mon, Sep 14, 5:36 PM · Restricted Project

Fri, Sep 11

phosek accepted D87195: [CMake][OpenMP] Simplify getting CUDA library directory.

LGTM

Fri, Sep 11, 11:01 AM · Restricted Project

Wed, Sep 9

phosek committed rGf7941d980918: [lit] Use correct variable name for libxml2 (authored by phosek).
[lit] Use correct variable name for libxml2
Wed, Sep 9, 10:05 PM
phosek committed rGc4d7536136b3: [CMake] Simplify CMake handling for libxml2 (authored by phosek).
[CMake] Simplify CMake handling for libxml2
Wed, Sep 9, 9:45 PM
phosek closed D84563: [CMake] Simplify CMake handling for libxml2.
Wed, Sep 9, 9:45 PM · Restricted Project
phosek updated the diff for D84563: [CMake] Simplify CMake handling for libxml2.
Wed, Sep 9, 9:43 PM · Restricted Project
phosek updated the diff for D84563: [CMake] Simplify CMake handling for libxml2.
Wed, Sep 9, 5:59 PM · Restricted Project

Tue, Sep 8

phosek added a comment to D86877: [Clang][Driver] Use full path to builtins in bare-metal toolchain.

It's not clear why couldn't we support per-target runtime directory? It uses the standard multiarch layout, so in you'd end up using a directory like [path/to/resource-dir]/lib/armv6m-unknown-eabi/libclang_rt.builtins.a. I'd find this more preferable for consistency with other targets, it'd also enable the use of runtimes build for baremetal.

Tue, Sep 8, 7:46 PM · Restricted Project

Mon, Sep 7

phosek accepted D87190: [CMake][TableGen] Remove dead CMake version checks.

LGTM

Mon, Sep 7, 12:16 AM · Restricted Project
phosek accepted D87191: [CMake][OpenMP] Remove old dead CMake code.

LGTM

Mon, Sep 7, 12:15 AM · Restricted Project
phosek accepted D87192: [CMake][Polly] Remove dead CMake code.

LGTM

Mon, Sep 7, 12:15 AM · Restricted Project
phosek accepted D87193: [CMake][TableGen] Simplify code by using list(TRANSFORM).

LGTM

Mon, Sep 7, 12:14 AM · Restricted Project

Wed, Sep 2

phosek added a comment to rG5201b962e895: [libc++] Re-apply the workaround for timespec_get not always being available in….

This broke our build with the following error:

FAILED: libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj 
/b/s/w/ir/k/staging/llvm_build/./bin/clang++ --target=aarch64-unknown-fuchsia --sysroot=/b/s/w/ir/k/cipd/sdk/arch/arm64/sysroot  -DHAVE___CXA_THREAD_ATEXIT_IMPL -DLIBCXXABI_USE_LLVM_UNWINDER -D_DEBUG -D_LIBCPP_DISABLE_EXTERN_TEMPLATE -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS -D_LIBCXXABI_BUILDING_LIBRARY -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/k/llvm-project/libcxxabi/include -I/b/s/w/ir/k/llvm-project/libunwind/include -I/b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include --target=aarch64-unknown-fuchsia -I/b/s/w/ir/k/cipd/sdk/pkg/fdio/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/k/staging/llvm_build/runtimes/runtimes-aarch64-unknown-fuchsia-bins=../staging/llvm_build/runtimes/runtimes-aarch64-unknown-fuchsia-bins -ffile-prefix-map=/b/s/w/ir/k/llvm-project/= -no-canonical-prefixes  -O2 -g  -fPIC   -nostdinc++ -Werror=return-type -W -Wall -Wchar-subscripts -Wconversion -Wmismatched-tags -Wmissing-braces -Wnewline-eof -Wunused-function -Wshadow -Wshorten-64-to-32 -Wsign-compare -Wsign-conversion -Wstrict-aliasing=2 -Wstrict-overflow=4 -Wunused-parameter -Wunused-variable -Wwrite-strings -Wundef -Wno-suggest-override -Wno-error -pedantic -fstrict-aliasing -funwind-tables -D_DEBUG -UNDEBUG -std=c++17 -MD -MT libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj -MF libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj.d -o libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj -c /b/s/w/ir/k/llvm-project/libcxxabi/src/cxa_exception_storage.cpp
In file included from /b/s/w/ir/k/llvm-project/libcxxabi/src/cxa_exception_storage.cpp:15:
In file included from /b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include/__threading_support:14:
In file included from /b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include/chrono:827:
/b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include/ctime:62:10: fatal error: 'sys/cdefs.h' file not found
#include <sys/cdefs.h>
         ^~~~~~~~~~~~~
1 error generated.
Wed, Sep 2, 3:10 PM

Tue, Sep 1

phosek accepted D87002: [compiler-rt] Don't build llvm-lit in RUNTIMES-BUILD.

LGTM

Tue, Sep 1, 11:06 PM · Restricted Project
phosek accepted D86877: [Clang][Driver] Use full path to builtins in bare-metal toolchain.

LGTM

Tue, Sep 1, 12:55 AM · Restricted Project

Mon, Aug 31

phosek committed rG3c7bfbd6831b: [CMake] Use find_library for ncurses (authored by phosek).
[CMake] Use find_library for ncurses
Mon, Aug 31, 8:07 PM
phosek closed D85820: Use find_library for ncurses.
Mon, Aug 31, 8:06 PM · Restricted Project, Restricted Project, Restricted Project
phosek accepted D85820: Use find_library for ncurses.

LGTM

Mon, Aug 31, 1:12 PM · Restricted Project, Restricted Project, Restricted Project
phosek added a comment to D86877: [Clang][Driver] Use full path to builtins in bare-metal toolchain.

This was discussed when LLVM_ENABLE_PER_TARGET_RUNTIME_DIR was introduced and there was a pushback against changing the driver behavior depending on the value of that option, so if we're going to reverse that decision for BareMetal, I think that deserves a broader discussion.

Mon, Aug 31, 1:10 PM · Restricted Project

Fri, Aug 28

phosek accepted D86822: [clang] Enable -fsanitize=thread on Fuchsia..

LGTM

Fri, Aug 28, 4:12 PM · Restricted Project

Thu, Aug 27

phosek added a comment to D77248: [llvm][IR] Add a new Constant (UnnamedFunc).

This is just a naming suggestion, but how about naming it unnamed_func (instead of unnamedfunc) so it's closer to unnamed_addr.

Thu, Aug 27, 4:49 PM · Restricted Project
phosek added a comment to D86521: Revert "Use find_library for ncurses".

I have suggested an alternative in https://reviews.llvm.org/D85820#2237357 to try and fix this forward and I was waiting for @haampie's response before landing this. I can implement that today if you're OK fixing this forward? I think it'd be easier to do that than reverting and then relanding all these changes again. @gkistanova how did you check the patch with http://lab.llvm.org:8011/builders/lld-perf-testsuite bot? Is there a way to do a presubmit check on that bot?

Thu, Aug 27, 1:20 PM · Restricted Project, Restricted Project, Restricted Project
phosek added a comment to rGda0592e4c8df: [libc++] Use CMake interface targets to setup benchmark flags.

@ldionne this is still failing and it's been several days now, can we revert this change?

Sorry folks, I was gone on vacation. I see you've reverted the change -- that's fine. I'll check it in again with a workaround for older CMakes.

In this case, the workaround will have to be that the libc++ benchmarks are *not* available on older CMakes. The issue is actually deeper than that, it is that the LLVM CMakeLists.txt is setting the CMAKE_CXX_STANDARD to 14 globally: https://github.com/llvm/llvm-project/blob/1c060aa988452508fcfc65357f4a6467f3f88cc1/llvm/CMakeLists.txt#L53.
There's no easy way to work around this AFAICT beyond using proper CMake target features. Note that we should stop setting the CMAKE_CXX_STANDARD (or any global flag really) in LLVM's CMakeLists.txt, but that's another story.

Thu, Aug 27, 11:17 AM
phosek added a comment to D85176: [Coverage] Enable emitting gap area between macros.

We started seeing assertion failure after rolling a toolchain that contains this change and git bisect identified this change. I have filed a bug with a reproducer as PR47324.

Thu, Aug 27, 1:14 AM · Restricted Project

Tue, Aug 25

phosek added a comment to D85820: Use find_library for ncurses.

@gkistanova It's true that this change has lead to more issues I could ever imagine, but I think the link you provided is the last remaining problem.

Pinging @phosek for a similar issue w.r.t. zlib: since https://reviews.llvm.org/D79219 zlib gets disabled on static builds of LLVM. The reason is find_package(ZLIB) finds a shared library, and check_symbol_exists(compress2 zlib.h HAVE_ZLIB) tries to statically link to that -- it can't so zlib is disabled. I guess that's a regression too?

What would be the best way forward @phosek? A quick fix for ncurses is to add a simple check_symbol_exists test too, but that would in practice just disable ncurses in static builds. And additionally we could find static libs by adding explicit names like libtinfo.a to find_library when LLVM_BUILD_STATIC=ON. This trick won't help for zlib though, the find_library stuff is hidden inside find_package, and there is no way to target static libs it seems.

Tue, Aug 25, 2:52 PM · Restricted Project, Restricted Project, Restricted Project

Mon, Aug 24

phosek committed rG2b3807d822c5: [CMake] Fix ncurses/zlib in LLVM_SYSTEM_LIBS for Windows GNU (authored by phosek).
[CMake] Fix ncurses/zlib in LLVM_SYSTEM_LIBS for Windows GNU
Mon, Aug 24, 11:06 PM
phosek closed D86434: Fix ncurses/zlib in LLVM_SYSTEM_LIBS for Windows GNU.
Mon, Aug 24, 11:06 PM · Restricted Project

Sat, Aug 22

phosek added a comment to D79219: [CMake] Simplify CMake handling for zlib.

@phosek in MSYS2 (targeting x86_64-w64-windows-gnu) Zlib works properly for LLVM 10 but with master I'm now seeing:

-- Constructing LLVMBuild project information
-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a
CMake Error at lib/Support/CMakeLists.txt:9 (string):
  string sub-command REGEX, mode REPLACE: regex "^(lib|)" matched an empty
  string.
Call Stack (most recent call first):
  lib/Support/CMakeLists.txt:226 (get_system_libname)

-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a was printed by my change to help debugging it.
FYI zlib_library is set here: https://github.com/llvm/llvm-project/blob/8e06bf6b3a2e8d25e56cd52dca0cf3ff1b37b5d1/llvm/lib/Support/CMakeLists.txt#L218

Sat, Aug 22, 7:23 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project

Aug 21 2020

phosek updated subscribers of rG31e5f7120bdd: [CMake] Simplify CMake handling for zlib.

@phosek do you know what is required to backport this as well as the ncurses detection to the LLVM 11 release branch?

Aug 21 2020, 1:36 PM
phosek accepted D86365: [compiler-rt] Disable ranlib when using libtool.

LGTM

Aug 21 2020, 1:33 PM · Restricted Project
phosek accepted D86367: [runtimes] Use llvm-libtool-darwin for runtimes build.

LGTM

Aug 21 2020, 1:32 PM · Restricted Project, Restricted Project
phosek accepted D86366: [runtimes] Remove TOOLCHAIN_TOOLS specialization.

LGTM

Aug 21 2020, 1:29 PM · Restricted Project

Aug 20 2020

phosek added a comment to D86331: Fix a cmake failure on Windows when LLVM_ENABLE_TERMINFO is set.

Wasn't this already addressed by https://github.com/llvm/llvm-project/commit/34fe9613dda3c7d8665b609136a8c12deb122382?

Aug 20 2020, 10:37 PM · Restricted Project
phosek accepted D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

LGTM

Aug 20 2020, 5:25 PM · Restricted Project
phosek added a comment to D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

I agree that the current situation is not optimal, I was never happy with the default case which is confusing for a lot of people I talked to. I would like to come up with a better solution, but I'm not sure if special casing the x86_64-apple-darwin is sufficient though. With Apple Silicon, I assume we're going to be building runtimes as universal binaries that target multiple architectures.

We could consider completely decoupling runtimes from targets. Originally, each runtime build was identified only by its target, so you'd use RUNTIMES_${target}_${variable} to set target-specific variables. That became insufficient when we introduced the support for multilib, so now you also have a name and you can use RUNTIMES_${target}+${name}_${variable}. We could switch to RUNTIMES_${name}_${variable} and it'd be the responsibility of the user to set the target explicitly if they want it (FWIW it's something we already have to do in our build anyway because of Windows where the CMake behavior when you set CMAKE_${LANG}_COMPILER_TARGET breaks the build). If not, they get the default behavior, matching the default case today. What do you think?

To clarify, what would be setting ${name} in this scenario?

Aug 20 2020, 3:29 PM · Restricted Project
phosek added a comment to D86313: [runtimes] Allow LLVM_BUILTIN_TARGETS to include Darwin.

I agree that the current situation is not optimal, I was never happy with the default case which is confusing for a lot of people I talked to. I would like to come up with a better solution, but I'm not sure if special casing the x86_64-apple-darwin is sufficient though. With Apple Silicon, I assume we're going to be building runtimes as universal binaries that target multiple architectures.

Aug 20 2020, 1:39 PM · Restricted Project
phosek added a comment to D86308: [CMake][compiler-rt][libunwind] Compile assembly files as ASM not C, unify workarounds.

I'm pretty sure add_asm_sources() has nothing to do. The ASM language is enabled by compiler-rt anyway and CMake can recognize the files as assembly anyway.

Aug 20 2020, 1:06 PM · Restricted Project, Restricted Project, Restricted Project
phosek committed rGd58fd4e52197: [compiler-rt] Compile assembly files as ASM not C (authored by phosek).
[compiler-rt] Compile assembly files as ASM not C
Aug 20 2020, 12:35 AM
phosek closed D85706: [compiler-rt] Compile assembly files as ASM not C.
Aug 20 2020, 12:35 AM · Restricted Project, Restricted Project

Aug 19 2020

phosek added a comment to rG67dfba96296b: [libc++] Provide std::aligned_alloc and std::timespec_get on Apple platforms.

@ldionne we've seen failures in various third party libraries like BoringSSL or ICU that were caused by this change. The problem is that timespec_get and aligned_alloc are only available if __DARWIN_C_LEVEL >= __DARWIN_C_FULL, but many existing libraries define _XOPEN_SOURCE in which case __DARWIN_C_LEVEL == _POSIX_C_SOURCE unless _DARWIN_C_SOURCE or _NONSTD_SOURCE are defined as well.

Aug 19 2020, 10:19 PM
phosek committed rG1ed1e16ab83f: [CMake] Fix an issue where get_system_libname creates an empty regex capture on… (authored by phosek).
[CMake] Fix an issue where get_system_libname creates an empty regex capture on…
Aug 19 2020, 2:35 PM
phosek closed D86245: Fix an issue where get_system_libname creates an empty regex capture on windows.
Aug 19 2020, 2:34 PM · Restricted Project
phosek accepted D86245: Fix an issue where get_system_libname creates an empty regex capture on windows.

LGTM

Aug 19 2020, 2:32 PM · Restricted Project
phosek added a comment to D86134: Fix OCaml build failure because of absolute path in system libs.

@haampie I landed your change but I'm seeing the following failure on sanitizer Windows bot:

CMake Error at lib/Support/CMakeLists.txt:223 (get_system_libname):
  get_system_libname Function invoked with incorrect arguments for function
  named: get_system_libname

The failing build is http://lab.llvm.org:8011/builders/sanitizer-windows/builds/68331

I'm going to revert the change, but I'm happy to reland it once we figure out what the problem is.

I also think this broke the Windows builds for Chrome: https://crbug.com/1119478.
CMake Error at lib/Support/CMakeLists.txt:9 (STRING):

STRING sub-command REGEX, mode REPLACE: regex "^()" matched an empty
string.

Call Stack (most recent call first):

lib/Support/CMakeLists.txt:218 (get_system_libname)

Might need zlib to repro.

Aug 19 2020, 1:19 PM · Restricted Project
phosek committed rG76bf26236f6f: [CMake] Always mark terminfo as unavailable on Windows (authored by phosek).
[CMake] Always mark terminfo as unavailable on Windows
Aug 19 2020, 11:52 AM
phosek closed D86234: [CMake] Always mark terminfo as unavailable on Windows.
Aug 19 2020, 11:52 AM · Restricted Project
phosek requested review of D86234: [CMake] Always mark terminfo as unavailable on Windows.
Aug 19 2020, 11:08 AM · Restricted Project
phosek added a comment to D86134: Fix OCaml build failure because of absolute path in system libs.

@haampie I landed your change but I'm seeing the following failure on sanitizer Windows bot:

Aug 19 2020, 11:01 AM · Restricted Project
phosek committed rG8e4acb82f71a: [CMake] Fix OCaml build failure because of absolute path in system libs (authored by phosek).
[CMake] Fix OCaml build failure because of absolute path in system libs
Aug 19 2020, 10:39 AM
phosek closed D86134: Fix OCaml build failure because of absolute path in system libs.
Aug 19 2020, 10:38 AM · Restricted Project
phosek committed rG495f91fd33d4: [CMake] Don't look for terminfo libs when LLVM_ENABLE_TERMINFO=OFF (authored by phosek).
[CMake] Don't look for terminfo libs when LLVM_ENABLE_TERMINFO=OFF
Aug 19 2020, 10:32 AM
phosek closed D86173: Don't look for terminfo libs when LLVM_ENABLE_TERMINFO=OFF.
Aug 19 2020, 10:32 AM · Restricted Project, Restricted Project

Aug 18 2020

phosek added a comment to D85384: [X86] Add basic support for -mtune command line option in clang.

This seems to have broken our Mac builders with the following error:

-- Testing: 25226 tests, 24 workers --
Testing:  0.. 10.. 20.
FAIL: Clang :: Frontend/ast-main.c (6834 of 25226)
******************** TEST 'Clang :: Frontend/ast-main.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   env SDKROOT="/" /b/s/w/ir/k/staging/llvm_build/bin/clang -emit-llvm -S -o /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp1.ll -x c - < /b/s/w/ir/k/llvm-project/clang/test/Frontend/ast-main.c
: 'RUN: at line 2';   env SDKROOT="/" /b/s/w/ir/k/staging/llvm_build/bin/clang -emit-ast -o /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp.ast /b/s/w/ir/k/llvm-project/clang/test/Frontend/ast-main.c
: 'RUN: at line 3';   env SDKROOT="/" /b/s/w/ir/k/staging/llvm_build/bin/clang -emit-llvm -S -o /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp2.ll -x ast - < /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp.ast
: 'RUN: at line 4';   diff /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp1.ll /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp2.ll
--
Exit Code: 1
Aug 18 2020, 9:45 PM · Restricted Project
phosek added a comment to D83154: clang: Add -fcoverage-prefix-map.

@keith do you plan on finishing this? I think we should change the name from -fcoverage-prefix-map to -fprofile-prefix-map, but otherwise it might be good to go.

Aug 18 2020, 7:02 PM · Restricted Project
phosek added a comment to D85056: [ELF] Add --keep-section to expose linkerscript KEEP directive as a linker flag.

I created https://sourceware.org/bugzilla/show_bug.cgi?id=26404 for the discussion on the syntax.

@christylee @phosek You may consider starting a thread on binutils@sourceware.org to expedite their processing:)

Aug 18 2020, 5:23 PM · Restricted Project
phosek accepted D86134: Fix OCaml build failure because of absolute path in system libs.

LGTM

Aug 18 2020, 3:55 PM · Restricted Project
phosek added a comment to D76482: [lld][ELF] Provide optional hidden symbols for build ID.

D76482 is an example use case for this feature.

Aug 18 2020, 3:36 PM · Restricted Project
phosek requested review of D86175: [profile][Fuchsia] Use build ID for the raw profile name.
Aug 18 2020, 3:35 PM
phosek accepted D86173: Don't look for terminfo libs when LLVM_ENABLE_TERMINFO=OFF.

LGTM

Aug 18 2020, 3:15 PM · Restricted Project, Restricted Project
phosek added inline comments to D86134: Fix OCaml build failure because of absolute path in system libs.
Aug 18 2020, 12:52 PM · Restricted Project

Aug 17 2020

phosek accepted D85820: Use find_library for ncurses.

LGTM

Aug 17 2020, 11:01 AM · Restricted Project, Restricted Project, Restricted Project
phosek added a comment to rGda0592e4c8df: [libc++] Use CMake interface targets to setup benchmark flags.

@ldionne this is still failing and it's been several days now, can we revert this change?

Aug 17 2020, 10:39 AM

Aug 15 2020

phosek added a comment to rGda0592e4c8df: [libc++] Use CMake interface targets to setup benchmark flags.

Note that this issue isn't manifesting with CMake 3.17 and newer.

Aug 15 2020, 1:54 PM

Aug 13 2020

phosek accepted D85936: [InterfaceStub] Remove unnecessary include dir from llvm/lib/InterfaceStub/CMakeLists.txt.

LGTM

Aug 13 2020, 2:53 PM · Restricted Project

Aug 12 2020

phosek added a comment to D85706: [compiler-rt] Compile assembly files as ASM not C.

One concern I have with this change is that we may not always consistently update CMAKE_ASM_FLAGS or set CMAKE_ASM_COMPILER. This wouldn't make a difference before, but it'll with this change. Can you check if these variables are being updated as needed?

Aug 12 2020, 2:25 PM · Restricted Project, Restricted Project
phosek added a comment to D85820: Use find_library for ncurses.

CMake already has FindCurses module so we could consider using find_package(Curses), unfortunately it doesn't define the imported target. I'd also prefer replacing HAVE_TERMINFO with LLVM_ENABLE_TERMINFO everywhere for consistency with zlib or libxml2 (see D84563).

Aug 12 2020, 12:59 PM · Restricted Project, Restricted Project, Restricted Project
phosek added inline comments to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.
Aug 12 2020, 12:54 PM · Restricted Project
phosek accepted D85678: [elfabi] Move elfabi related code to InterfaceStub library.

LGTM

Aug 12 2020, 12:33 PM · Restricted Project

Aug 11 2020

phosek added a comment to D85678: [elfabi] Move elfabi related code to InterfaceStub library.

I'd be fine with InterfaceStub as a name for the library.

@phosek @haowei Are there any plans in the near term to unify any of the TBD work with InterfaceStubs/elftapi?

@ributzka @cishida

Aug 11 2020, 11:32 PM · Restricted Project