Page MenuHomePhabricator

jfb (JF Bastien)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 27 2014, 9:36 AM (322 w, 1 d)

Recent Activity

Today

jfb added a comment to D77168: Add a flag to debug automatic variable initialization.

I'm not sure this is a good idea at all. We want to keep the codepath as simple as possible to avoid introducing bugs. If a codebase sees a crash then it's easier to bisect one function at a time than doing something like this. I'd much rather see bisection using pragma to apply the uninitialized attribute to multiple declarations.

Tue, Mar 31, 3:59 PM · Restricted Project

Fri, Mar 27

jfb added a comment to D68115: Zero initialize padding in unions.

It sounds like we are looking for -fzero-union-padding. That's been where the discussion has left off twice for months.

I believe the state of Clang prior to this patch is actually wrong.

Fri, Mar 27, 2:52 PM · Restricted Project
jfb added a comment to D68115: Zero initialize padding in unions.

What's the verdict then?

Fri, Mar 27, 12:01 PM · Restricted Project

Mon, Mar 23

jfb accepted D76528: [clang codegen] Clean up handling of vectors with trivial-auto-var-init..

Thanks!

Mon, Mar 23, 7:04 PM · Restricted Project
jfb added a comment to D76528: [clang codegen] Clean up handling of vectors with trivial-auto-var-init..

The test files I added checked initialization had stores to each padding location, I think that's what would be needed here as well.

Mon, Mar 23, 3:48 PM · Restricted Project
jfb accepted D76323: [AST] Fix handling of long double and bool in __builtin_bit_cast.
Mon, Mar 23, 3:16 PM

Fri, Mar 20

jfb added a comment to D76528: [clang codegen] Clean up handling of vectors with trivial-auto-var-init..

Oh good catch.

Fri, Mar 20, 4:50 PM · Restricted Project
jfb requested changes to D76517: [MegreSimilarFunctions] D52896, D52898 and D52966 merged into LLVM trunk.

I've provided a large amount of feedback on prior iterations of this, and would like to see it addressed.

Fri, Mar 20, 11:55 AM · Restricted Project, Restricted Project
jfb added a comment to D76323: [AST] Fix handling of long double and bool in __builtin_bit_cast.

Maybe you should test nullptr as well, given that it's all padding?

Fri, Mar 20, 9:44 AM

Fri, Mar 13

jfb added a comment to D76077: [ARM] Add __bf16 as new Bfloat16 C Type.

I don't understand why you wouldn't add a new IR type for this; doing so should be totally mechanical.

We had a few reasons for doing it this way.

By adding a new IR type we would need to consider calling conventions, and IR optimizations for what is essentially an opaque storage-only type.

IR optimizations should just fall out; the code in APFloat should work for arbitrary FP semantics.

Calling something a "storage-only" type does not get you out of worrying about calling conventions at the ABI level. You can forbid passing your type as an argument or result directly, but structs containing your type as a field can still be passed around, and the behavior for that needs to be defined.

Bfloat has no soft ABI, and all the supported operations can be handled through intrinsics (in a later patch, removed here due to bloat). If we were to add a new IR type, then we would need to handle many operations which would extend beyond what the type is supported for. For instance in GCC we had to add a new mode in RTL to handle inline memcopy.

I don't know why having a soft ABI makes a difference. If the type only works on certain platforms, then it can't be used on other platforms.

Fri, Mar 13, 12:23 PM · Restricted Project
jfb added inline comments to D75960: [libc++] Implement C++20's P0476r2: std::bit_cast.
Fri, Mar 13, 11:17 AM · Restricted Project

Thu, Mar 12

jfb added a reviewer for D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support: jfb.
Thu, Mar 12, 11:23 AM · Restricted Project, Restricted Project
jfb added a reviewer for D76077: [ARM] Add __bf16 as new Bfloat16 C Type: jfb.
Thu, Mar 12, 11:23 AM · Restricted Project

Mon, Mar 9

jfb committed rG8fc9eea43a94: Test that volatile load type isn't changed (authored by jfb).
Test that volatile load type isn't changed
Mon, Mar 9, 11:52 AM
jfb closed D75644: Test that volatile load type isn't changed.

8fc9eea43a94d83e46614eee47b069e25848fedb

Mon, Mar 9, 11:52 AM · Restricted Project

Fri, Mar 6

jfb added a comment to D75505: [InstCombine] Enhance cast-of-load->load-of-bitcast fold to handle multiple identical casts.

Can you add a few tests for this specifically, both with identical subsequent bitcasts as well as with mixed ones?

Fri, Mar 6, 10:28 AM · Restricted Project

Thu, Mar 5

jfb updated the diff for D75644: Test that volatile load type isn't changed.

Add comment

Thu, Mar 5, 3:21 PM · Restricted Project
jfb added a comment to D75644: Test that volatile load type isn't changed.

Odd, the previous update only had the second commit. I've squashed all 3 commits now, so it's all here :)

Thu, Mar 5, 3:21 PM · Restricted Project

Wed, Mar 4

jfb updated the diff for D75644: Test that volatile load type isn't changed.

Add i64/double test, as well as ptr/int

Wed, Mar 4, 4:53 PM · Restricted Project
jfb created D75644: Test that volatile load type isn't changed.
Wed, Mar 4, 2:05 PM · Restricted Project
jfb added inline comments to D75505: [InstCombine] Enhance cast-of-load->load-of-bitcast fold to handle multiple identical casts.
Wed, Mar 4, 11:15 AM · Restricted Project

Tue, Mar 3

jfb added inline comments to D75505: [InstCombine] Enhance cast-of-load->load-of-bitcast fold to handle multiple identical casts.
Tue, Mar 3, 1:47 PM · Restricted Project
jfb added a comment to D75505: [InstCombine] Enhance cast-of-load->load-of-bitcast fold to handle multiple identical casts.

Please add tests for volatile loads.

Tue, Mar 3, 12:37 PM · Restricted Project
jfb added a comment to D75436: [profile] Remove fork management from code coverage.

Doing fork will still share an output file, right? Doe we currently end up interleaving content from both forks, or does one of them overwrite the other entirely?
I understand that crashing is undesirable, but wrong data isn't really any better.

Tue, Mar 3, 10:56 AM · Restricted Project, Restricted Project

Mon, Mar 2

jfb added a comment to D75266: SROA: Don't drop atomic load/store alignments (PR45010).
In D75266#1898238, @jfb wrote:

I meant compare and exchange, as well as RMW.

It doesn't look like SROA handles cmpxchg or atomicrmw instructions.

Mon, Mar 2, 10:41 AM · Restricted Project
jfb added a reviewer for D74324: Tools emit the bug report URL on crash: arphaman.
Mon, Mar 2, 10:41 AM · Restricted Project, Restricted Project

Feb 28 2020

jfb added a comment to D75266: SROA: Don't drop atomic load/store alignments (PR45010).

I meant compare and exchange, as well as RMW.

Feb 28 2020, 8:42 AM · Restricted Project

Feb 27 2020

jfb accepted D74918: Add method to TargetInfo to get CPU cache line size.
Feb 27 2020, 1:30 PM · Restricted Project
jfb added a reviewer for D75236: [APFloat] Overload unary operator-: scanon.
Feb 27 2020, 9:36 AM · Restricted Project
jfb added a reviewer for D75237: [APFloat] Overload comparison operators: scanon.
Feb 27 2020, 9:36 AM · Restricted Project
jfb added inline comments to D75266: SROA: Don't drop atomic load/store alignments (PR45010).
Feb 27 2020, 9:28 AM · Restricted Project

Feb 26 2020

jfb added a comment to D68480: Implementation of C++20's P1135R6 for libcxx.

@leonardchan @phosek Is there a way that you could add a libc++ builder that runs freestanding?

We have it as a TODO but can bump it in priority.

Agreed, it seems like we want to figure out what individual freestanding subsets can "carve out" of regular C++, and how to do so cleanly. "time" seems like an easy thing to carve out, but it shouldn't just be done here, it should be part of the configuration header, applied consistently through libc++, and extensively tested. Ideally it wouldn't just be a builder: you'd have a directory which tests freestanding configuration options (i.e. "can I include atomic without time support?").

How simple would it be to carve specifically time.h out? I'm unfamiliar with libcxx internals + configurations. I'm also not insisting that progress on this patch be halted, but I'd like to know if there's anything we could do at least now to provide a workaround for freestanding targets,

Feb 26 2020, 2:11 PM · Restricted Project
jfb added a comment to D68480: Implementation of C++20's P1135R6 for libcxx.
In D68480#1893987, @jfb wrote:

Hi, a bisect shows that this patch seems to cause time.h to not be found for us:

[...]

Do you know if this is a known or unintended side effect of this patch? I don't think we changed anything on our side that would've caused this.

Builder: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8887404211928925456?

Is zircon freestanding, and not providing chrono (which then includes time.h)? That would explain the issue, we'd need to fix freestanding atomic by not exposing time-related stuff if time.h ins't available.

So, actually, I think we need to have a discussion about the support of Freestanding in libc++. We should either implement it properly and have testers for it, or stop pretending that we do. Right now all I see us doing is adding complexity to our code to fix stuff that breaks on user configurations we don't know about, understand or test. As someone who has to maintain the code with that complexity, this concerns me.

And I do care about Freestanding -- I think it's important and we should support it. But I also think that we should not do it halfway like right now. In the current state of things, I literally have no way to ensure that we won't break that configuration again tomorrow. @leonardchan @phosek Is there a way that you could add a libc++ builder that runs freestanding?

Feb 26 2020, 10:44 AM · Restricted Project
jfb added a comment to D75183: [libcxx] Guard C++20 atomic type aliases.

Actually: how are your locks implemented to support atomic? You disable interrupts? I think you want to contribute a version of atomic which does this, instead of saying "nothing is lock free, use locks" and then doing all the magic in the locks.

Feb 26 2020, 10:25 AM · Restricted Project
jfb added a comment to D68480: Implementation of C++20's P1135R6 for libcxx.

Hi, a bisect shows that this patch seems to cause time.h to not be found for us:

[1958/45287] CXX kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o
FAILED: kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o
../../../recipe_cleanup/clangEPCywe/bin/clang++ -MD -MF kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o.d -o kernel-x64-clang/obj/kernel/lib/libc/libc.snprintf.cc.o -DTOOLCHAIN_VERSION=/b/s/w/ir/k/recipe_cleanup/clangEPCywe/bin -DZX_ASSERT_LEVEL=2 -DWITH_FRAME_POINTERS=1 -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -DKERNEL_BASE=0xffffffff80100000 -DSMP_MAX_CPUS=32 -D_KERNEL -DLK -DENABLE_PANIC_SHELL -DWITH_DEBUG_LINEBUFFER -DZIRCON_TOOLCHAIN -DLK_DEBUGLEVEL=2 -DWITH_KERNEL_PCIE -DKERNEL_RETPOLINE=1 -DWITH_UNIFIED_SCHEDULER=1 -DSCHEDULER_TRACING_LEVEL=0 -DARCH_X86 -DKERNEL_LOAD_OFFSET=0x00100000 -D_LIBCPP_DISABLE_EXTERN_TEMPLATE -I../../zircon/kernel/include -I../../zircon/kernel/lib/libc/include -I../../zircon/kernel/lib/ktl/include -I../../zircon/kernel/lib/io/include -I../../zircon/kernel/lib/heap/include -I../../zircon/system/ulib/lazy_init/include -I../../zircon/system/ulib/lockdep/include -I../../zircon/system/ulib/ffl/include -I../../zircon/kernel/vm/include -I../../zircon/kernel/lib/user_copy/include -I../../zircon/system/ulib/zircon-internal/include -I../../zircon/kernel/lib/ktrace/include -I../../zircon/system/ulib/fbl/include -I../../zircon/kernel/lib/fbl/include -I../../zircon/system/public -I../../zircon/kernel/arch/x86/include -I../../zircon/system/ulib/bitmap/include -I../../zircon/kernel/arch/x86/page_tables/include -I../../zircon/system/ulib/hwreg/include -I../../zircon/kernel/lib/heap/include -I../../zircon/kernel/lib/io/include -I../../zircon/kernel/lib/ktl/include -idirafter ../../zircon/kernel/lib/libc/limits-dummy -fno-common --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out/default.zircon=. -ffile-prefix-map=/b/s/w/ir/k/fuchsia/out=.. -ffile-prefix-map=/b/s/w/ir/k/fuchsia=../.. -no-canonical-prefixes -O2 -g3 -Wall -Wextra -Wno-unused-parameter -Wno-address-of-packed-member -Wnewline-eof -Wno-unknown-warning-option -Wno-c99-designator -Wno-int-in-bool-context -Wno-range-loop-analysis -fno-omit-frame-pointer -ffunction-sections -fdata-sections -Wthread-safety -Wimplicit-fallthrough -fvisibility=hidden -ftrivial-auto-var-init=pattern -Werror -Wno-error=deprecated-declarations -fpie -mretpoline -mretpoline-external-thunk -ffreestanding -include ../../zircon/kernel/include/hidden.h -fno-unwind-tables -mno-red-zone -Wformat=2 -Wvla -mno-red-zone -msoft-float -mno-mmx -mno-sse -mno-sse2 -mno-3dnow -mno-avx -mno-avx2 -mcmodel=kernel -fdata-sections -Wno-gnu-string-literal-operator-template -std=c++17 -Wconversion -Wno-sign-conversion -Wextra-semi -Wno-deprecated-copy -Wno-non-c-typedef-for-linkage -ftemplate-backtrace-limit=0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -faligned-new=8 -fno-exceptions -c ../../zircon/kernel/lib/libc/snprintf.cc
In file included from ../../zircon/kernel/lib/libc/snprintf.cc:10:
In file included from ../../zircon/kernel/lib/ktl/include/ktl/algorithm.h:10:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/algorithm:643:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/memory:666:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/atomic:550:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/__threading_support:14:
In file included from ../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/chrono:827:
../../../recipe_cleanup/clangEPCywe/bin/../include/c++/v1/ctime:49:10: fatal error: 'time.h' file not found
#include <time.h>

Do you know if this is a known or unintended side effect of this patch? I don't think we changed anything on our side that would've caused this.

Builder: https://ci.chromium.org/p/fuchsia/builders/ci/clang_toolchain.fuchsia-x64-debug-subbuild/b8887404211928925456?

Feb 26 2020, 10:25 AM · Restricted Project
jfb added a comment to D75183: [libcxx] Guard C++20 atomic type aliases.

Please add extensive tests, this will absolutely break in the future without them.

Feb 26 2020, 10:13 AM · Restricted Project

Feb 20 2020

jfb added inline comments to D74918: Add method to TargetInfo to get CPU cache line size.
Feb 20 2020, 2:02 PM · Restricted Project

Feb 18 2020

jfb added a comment to D74361: [Clang] Undef attribute for global variables.

I'm not sure if it's a good idea to be re-using the existing attribute like this. It's not that unreasonable, I guess, but I'd like to hear JF's thoughts.

Feb 18 2020, 9:30 AM · Restricted Project

Jan 31 2020

jfb accepted D73390: RNG: Take pass name as argument instead of pass pointer..
Jan 31 2020, 10:54 AM · Restricted Project

Jan 30 2020

jfb accepted D67399: [ARM] Follow AACPS for preserving number of loads/stores of volatile bit-fields.

I'm happy with this change since it's opt-in. Thanks!

Jan 30 2020, 9:39 AM · Restricted Project

Jan 27 2020

jfb updated subscribers of D68480: Implementation of C++20's P1135R6 for libcxx.
Jan 27 2020, 10:48 AM · Restricted Project

Jan 20 2020

jfb added inline comments to D72995: [MLIR] LLVM Dialect: add llvm.cmpxchg and improve llvm.atomicrmw custom parser.
Jan 20 2020, 4:09 PM · Restricted Project
jfb added inline comments to D72995: [MLIR] LLVM Dialect: add llvm.cmpxchg and improve llvm.atomicrmw custom parser.
Jan 20 2020, 3:41 PM · Restricted Project

Jan 17 2020

jfb accepted D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw.
In D72741#1827039, @jfb wrote:

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR.

Maybe what is causing confusion is that we also have the MLIR standard dialect, which is more of a new IR itself. We are actually quite careful about things going in the standard dialect and it isn't bound by LLVM IR (other that we try to think about how to map down to LLVM).

Another way to put it is that the LLVM Dialect is only a layer intended to be hiding the LLVM IR Builders: while in MLIR you lower down to the LLVM Dialect instead of emitting directly LLVM IR. The reason is that this composes better (you can lower progressively to LLVM and not as a one shot conversion). So the LLVM dialect "embeds" the LLVM IR inside MLIR.

Jan 17 2020, 11:58 AM · Restricted Project
jfb added a comment to D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw.

I don't think this commit should be about making changes to LLVM IR. I can do that in a followup, but this commit should be in sync with whatever LLVM IR is today. There seems to be consensus with this approach.

Jan 17 2020, 11:11 AM · Restricted Project
jfb requested changes to D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw.

@ftynse I believe this is ready to go, anything else you'd like?

Also, I don't yet have committer access, so I'll need some help getting it landed.

Jan 17 2020, 10:51 AM · Restricted Project

Jan 16 2020

jfb updated subscribers of D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw.
Jan 16 2020, 9:44 AM · Restricted Project

Jan 14 2020

jfb updated subscribers of D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw.
Jan 14 2020, 4:43 PM · Restricted Project
jfb updated subscribers of D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw.
Jan 14 2020, 4:23 PM · Restricted Project
jfb added inline comments to D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw.
Jan 14 2020, 4:23 PM · Restricted Project

Jan 13 2020

jfb added a comment to D71600: PowerPC 32-bit - forces 8 byte lock/lock_free decisions at compiled time.

I am still confused why you need the special rule for __atomic_is_lock_free (GCC/clang) and __c11_atomic_is_lock_free (clang).

https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c#L7300-L7314 GCC __atomic_is_lock_free expands to either 1 or a library call to __atomic_is_lock_free, never 0. How did FreeBSD get around libatomic when it was still using GCC? In clang, We can probably extend the semantics of __c11_atomic_is_lock_free and use clang::TargetInfo::getMaxAtomicPromoteWidth, but I'll make more research in this area.

Can you be elaborate how do __atomic_is_lock_free/__c11_atomic_is_lock_free pull in libatomic.{a,so} dependency on FreeBSD powerpc?

On FreeBSD 11.x/12.x (GCC4), libatomic can be installed by user through "gcc9" ports package. The library is not shipped with the base system and it's not required to compile the base system (world + kernel) since nothing depends on 64 bit atomics an external call so __atomic_is_lock_free is never made.

The problem appeared when I switched to clang in a new ISO, this added an undesired external libatomic dependency to build FreeBSD base sources (world): clang was emitting a call to external __atomic_is_lock_free implementation when building clang itself from base sources. (user would need to install gcc9 to be able to build FreeBSD base source using clang, it's not acceptable.

Jan 13 2020, 10:16 AM · Restricted Project, Restricted Project, Restricted Project
jfb updated subscribers of D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth.

This changes the expression to a constant expression as well, right? You should point this out in the commit message.

Jan 13 2020, 10:16 AM · Restricted Project
jfb updated subscribers of D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth.
Jan 13 2020, 10:16 AM · Restricted Project

Jan 8 2020

jfb added a comment to D68115: Zero initialize padding in unions.

@vitalybuka could we move this patch forward?

Jan 8 2020, 2:23 PM · Restricted Project
jfb added a comment to D70326: [docs] LLVM Security Group and Process.

We (Microsoft) are interested in participating in this process.

Jan 8 2020, 10:30 AM · Restricted Project

Jan 7 2020

jfb added a comment to D70326: [docs] LLVM Security Group and Process.

I've not read this in detail or followed the list, but I wanted to add that I believe it's important that we have some form of public acknowledgement for the people that have reported security vulnerabilities in there as well.

Jan 7 2020, 9:38 PM · Restricted Project
jfb added a comment to D70326: [docs] LLVM Security Group and Process.

We should explicitly state that patches to LLVM sent to the group are subject to the standard LLVM developer policy/license. This is important so members of the security group can use any patches.

We should prominently state that all messages and attachments will be publicly disclosed after any embargo expires. This is important so issue reporters don't send code under NDAs/etc.

Jan 7 2020, 9:38 PM · Restricted Project
jfb updated the diff for D70326: [docs] LLVM Security Group and Process.
  • Address feedback
Jan 7 2020, 9:30 PM · Restricted Project

Jan 5 2020

jfb updated subscribers of D72240: Implement C++20 std::atomic_ref and test.
Jan 5 2020, 9:03 PM · Restricted Project

Jan 3 2020

jfb added a comment to D70910: [compiler-rt] Add a critical section when flushing gcov counters.

OK that helps a bit. I still don't understand though: why do you only need to protect flush and nothing else? You're protecting specific variables from being accesses concurrently only though flush, and not through the other paths that access those variables. Are these variables never accessed concurrently from these other functions?

Jan 3 2020, 1:49 PM · Restricted Project, Restricted Project

Dec 19 2019

jfb updated subscribers of D71726: Let clang atomic builtins fetch add/sub support floating point types.
Dec 19 2019, 2:53 PM
jfb updated subscribers of D71726: Let clang atomic builtins fetch add/sub support floating point types.

This generally seems fine. Does it work on most backends? I want to make sure it doesn't fail in backends :)

Dec 19 2019, 2:53 PM

Dec 12 2019

jfb committed rGd779dda7d0f9: Fix index while building following bitstream reader API update (authored by jfb).
Fix index while building following bitstream reader API update
Dec 12 2019, 2:04 PM
jfb committed rGaf934467296e: Fix APINotes following bitstream reader API update (authored by jfb).
Fix APINotes following bitstream reader API update
Dec 12 2019, 2:04 PM
jfb added a comment to D70910: [compiler-rt] Add a critical section when flushing gcov counters.

@jfb, sorry I thought my commit message was clear enough.
So when coverage is enabled a call to __gcov_flush is inserted just before forkcall:
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp#L659
So if the parent process is forked in different threads then __gcov_flush is called from different threads.
And so different global variables are accessed asynchronously:
https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/profile/GCDAProfiling.c#L104-L108
Initially we saw crashes in our CI: https://bugzilla.mozilla.org/show_bug.cgi?id=1599436
The above test case leads to multiple errors: double-free, "cannot merge previous GCDA file..."
And in debbuging firefox, I saw output_file == NULL (https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/profile/GCDAProfiling.c#L603) where it mustn't.

Dec 12 2019, 11:19 AM · Restricted Project, Restricted Project
jfb added a comment to D70910: [compiler-rt] Add a critical section when flushing gcov counters.

@jfb sorry but it fixes issues with fork:

  • we had a lot of crashes in Firefox CI when dumping the GCDAs and so I tried this patch on the CI and no more weird crashes.
  • and a minimal test case to reproduce it: ` #include <iostream> #include <thread> #include <vector> #include <unistd.h>

    void foo() { fork(); }

    int main() { std::vector<std::thread> pool;

    for (int i = 0; i < 10; i++) { pool.emplace_back(std::thread(foo)); }

    for (auto & t : pool) { t.join(); }

    return 0; } `

    If I compile and run it (clang++-9 -std=c++11 foo.cpp -oo -lpthread --coverage && ./o) I get systematically a SEGFAULT and with the patch no more crashes. The problem is not with flush_fn_list itself but it's when the functions in the list (gcda dumper) are called concurrently.
Dec 12 2019, 9:55 AM · Restricted Project, Restricted Project

Dec 9 2019

jfb added a comment to D70910: [compiler-rt] Add a critical section when flushing gcov counters.

When you say "when the process is forked in different threads" you mean threads, not fork, right? Because this won't fix any issues you're seeing with fork.

Dec 9 2019, 8:57 PM · Restricted Project, Restricted Project
jfb updated the summary of D49091: Warn about usage of __has_include/__has_include_next in macro expansions.
Dec 9 2019, 11:02 AM · Restricted Project

Dec 6 2019

jfb added a reviewer for D71128: [NVPTX][FIX] Expand atomics we cannot handle natively in the ISA: __simt__.
Dec 6 2019, 9:55 AM · Restricted Project
jfb added a comment to D71091: Make sure that the implicit arguments for direct methods have been setup.

You should upload patches with context :)

Dec 6 2019, 9:08 AM · Restricted Project

Dec 5 2019

jfb added a reviewer for D66822: Hardware cache line size builtins: __simt__.
Dec 5 2019, 9:26 AM · Restricted Project

Nov 16 2019

jfb updated the summary of D70326: [docs] LLVM Security Group and Process.
Nov 16 2019, 2:36 PM · Restricted Project

Nov 15 2019

jfb created D70326: [docs] LLVM Security Group and Process.
Nov 15 2019, 10:59 AM · Restricted Project

Nov 13 2019

jfb added inline comments to D63131: arm64_32: implement the desired ABI for the ILP32 triple..
Nov 13 2019, 1:56 PM · Restricted Project

Nov 8 2019

jfb accepted D63131: arm64_32: implement the desired ABI for the ILP32 triple..

Minor comments, but otherwise LGTM.

Nov 8 2019, 4:01 AM · Restricted Project

Oct 21 2019

jfb added a comment to D66046: Add new tautological compare warning for bitwise-or with a non-zero constant.

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

Oct 21 2019, 2:04 PM · Restricted Project

Oct 17 2019

jfb accepted D69148: Disable exit-on-SIGPIPE in lldb.

More of an FYI, @jordan_rose might be interested in this.

Oct 17 2019, 4:59 PM · Restricted Project, Restricted Project
jfb added inline comments to D69148: Disable exit-on-SIGPIPE in lldb.
Oct 17 2019, 4:21 PM · Restricted Project, Restricted Project

Oct 16 2019

jfb added a comment to D68964: cmake/modules/CheckAtomic.cmake: catch false positives in RISC-V.

This is bogus -- the RISCV GCC implementation is broken. It should not be mixing libatomic function calls with direct atomics.

Oct 16 2019, 10:00 AM · Restricted Project

Oct 14 2019

jfb accepted D68964: cmake/modules/CheckAtomic.cmake: catch false positives in RISC-V.

This is pretty brittle... but 🤷‍♂️

Oct 14 2019, 9:02 PM · Restricted Project

Sep 27 2019

jfb added a comment to D68115: Zero initialize padding in unions.

The entire point of this feature is to add guardrails to the language. What do people expect in the real world? Is there a cost to meeting these expectations? If we put the pattern (0x00 or 0xaa) in the technically undef space, what comes out?

Sep 27 2019, 7:26 PM · Restricted Project
jfb accepted D67436: CodeGen: set correct result for atomic compound expressions.

Separately, does this do floating-point add / sub properly? We added them too C++20.

Sep 27 2019, 9:28 AM · Restricted Project

Sep 13 2019

jfb added a comment to D67399: [ARM] Follow AACPS for preserving number of loads/stores of volatile bit-fields.

Indeed our main concern is regarding the access widths of loads. As mentioned by @rjmccall, most volatile bitfields are used to perform memory mapped I/O, and some hardware only support them with a specific access width.
The spurious load I am more than glad to leave it disable behind a command flag, so it will only appear if the user requests it. See that volatile accesses might have side effects, and for example, an I/O read counter holding an odd number could define that the data is still being processed.

Sep 13 2019, 8:48 AM · Restricted Project

Sep 12 2019

jfb accepted D64146: [Clang Interpreter] Initial patch for the constexpr interpreter.

Sounds like this is ready to land again! Thanks for fixing everything.

Sep 12 2019, 3:22 PM · Restricted Project, Restricted Project

Sep 10 2019

jfb added inline comments to D67399: [ARM] Follow AACPS for preserving number of loads/stores of volatile bit-fields.
Sep 10 2019, 9:01 AM · Restricted Project
jfb updated subscribers of D67399: [ARM] Follow AACPS for preserving number of loads/stores of volatile bit-fields.
Sep 10 2019, 9:00 AM · Restricted Project

Sep 6 2019

jfb added a comment to D67248: [InstCombine] pow(x, +/- 0.0) -> 1.0.

Sorry - I didn't see this review, only the bug report that came in today:
https://bugs.llvm.org/show_bug.cgi?id=43233

I made an identical code change, but I didn't include as many tests. Feel free to commit those or let me know if you want me to.

Sep 6 2019, 9:31 AM · Restricted Project
jfb committed rG52614dfc7fd0: [InstCombine] pow(x, +/- 0.0) -> 1.0 (authored by jfb).
[InstCombine] pow(x, +/- 0.0) -> 1.0
Sep 6 2019, 9:28 AM
jfb committed rL371224: [InstCombine] pow(x, +/- 0.0) -> 1.0.
[InstCombine] pow(x, +/- 0.0) -> 1.0
Sep 6 2019, 9:25 AM
jfb added a comment to D67248: [InstCombine] pow(x, +/- 0.0) -> 1.0.
Sep 6 2019, 9:25 AM · Restricted Project
jfb closed D67248: [InstCombine] pow(x, +/- 0.0) -> 1.0.
Sep 6 2019, 9:25 AM · Restricted Project

Sep 5 2019

jfb updated the diff for D67248: [InstCombine] pow(x, +/- 0.0) -> 1.0.
  • Address arsenm's comments
Sep 5 2019, 5:39 PM · Restricted Project
jfb created D67248: [InstCombine] pow(x, +/- 0.0) -> 1.0.
Sep 5 2019, 5:27 PM · Restricted Project

Sep 4 2019

jfb added inline comments to D66397: [Diagnostics] Improve -Wxor-used-as-pow.
Sep 4 2019, 3:04 PM · Restricted Project
jfb committed rL370958: Request commit access for jfb.
Request commit access for jfb
Sep 4 2019, 12:17 PM

Sep 3 2019

jfb requested changes to D66822: Hardware cache line size builtins.

Sorry for the delayed response, I was on vacation. Thanks for tackling it!

Sep 3 2019, 3:20 PM · Restricted Project
jfb added a comment to D67023: Diagnose use of ATOMIC_VAR_INIT.
In D67023#1654070, @jfb wrote:

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

P0883 has not been adopted, that I can tell (it has strong support, but isn't [expected to be] a part of C++2a)?

Sep 3 2019, 3:05 PM

Sep 1 2019

jfb added a comment to D67023: Diagnose use of ATOMIC_VAR_INIT.

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

Sep 1 2019, 3:15 PM

Aug 30 2019

jfb added a comment to D67023: Diagnose use of ATOMIC_VAR_INIT.

Is atomic initialization now correct in all modes (including C++) without this macro? I don’t think we should diagnose until such a time because some code uses to macro to be portably correct. IIRC we only ended up fixing C++ in 20 with Nico’s paper (after Olivier and I failed repeatedly to do so.

Aug 30 2019, 5:26 PM
jfb added inline comments to D66959: Update libc++ release notes.
Aug 30 2019, 1:28 AM · Restricted Project