In D76389#1951135, @arsenm wrote:

Commit message should say only if?

In D76893#1945285, @MaskRay wrote:

In D76893#1945243, @leonardchan wrote:

LGTM. Thanks for the fix. I'll go ahead and submit this.

You should have done git commit --amend --author='Paul Robinson <...>' to credit this change properly to Paul.

LGTM. Thanks for the fix. I'll go ahead and submit this.

Hi. This patch seems to cause the undefined symbol error we're seeing when linking clang:

LGTM. Thanks for the fix!

In D76651#1942163, @Dor1s wrote:

@leonardchan I've committed https://github.com/llvm/llvm-project/commit/6d0488f75bb2f37bcfe93fc8f59f6e78c9a0c939, that should help, I'll monitor the buildbot.

Hi. I think this patch might be leading to the linker error we're seeing:

In D74619#1936336, @pratlucas wrote:

Hi @leonardchan ,

I've double-checked the Neon intrinsics reference and it indeed confirms that the only allowed value for the lane argument for vdupq_lane_f64 is 0:

Argument Preparation

vec → Vn.1D 
0 << lane << 0

(https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics?search=vdupq_lane_f64)

I believe the observed behaviour is expected.

In D76119#1936439, @junparser wrote:

hi, @leonardchan, It seems that you disabled lifetime insertion at https://reviews.llvm.org/D63153. Is there any other reason to disable it beyond fix testcases? Also, could you please review this? we need lifetime check to prevent potential ‘use-after-free’ issue described at https://reviews.llvm.org/D75664

Actually this may not need a reproducer since the error seems straightforward with just calling a builtin function. The main issue is that before this patch, something like

Hi, a bisect seems to show https://reviews.llvm.org/rGf56550cf7f12b581a237b48a7f4d8b6682d45a09 is causing us to see the following error:

Putting this on hold for now. Although this implementation works for ASan, it would be have to be repeated for other tools like SourceBasedCoverage or other sanitizers.

Oh, I think you'll need to add it to llvm/cmake/modules/LLVMExternalProjectUtils.cmake also:

Is it expected that this patch could increase binary size? I'm seeing about a 9kB total increase in fuchsia ZBIs between clang with this patch and the one before it.

In D74825#1927964, @arsenm wrote:

In D74825#1927949, @leonardchan wrote:

In D74825#1927673, @arsenm wrote:

In D74825#1927641, @leonardchan wrote:

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

The pass isn't supposed to even run for CPU targets

I think this pass is also running for non-GPU targets. I'm seeing this pass run on x86_64, aarch64, and riscv64 when building a toolchain for those targets.

https://github.com/llvm/llvm-project/blob/a4cde9ad7b6f1a4cfef228f6cf2fc4911bf24c77/llvm/lib/Passes/PassBuilder.cpp#L436 seems to add it to the new PM default function pipeline that I think runs as long as optimizations are available.

That doesn't match what the comment says, or the old PM does (which does createSpeculativeExecutionIfHasBranchDivergencePassz)

In D74825#1927673, @arsenm wrote:

In D74825#1927641, @leonardchan wrote:

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

The pass isn't supposed to even run for CPU targets

Is it expected for binary size increases to result from this? Between the commit for this patch and the commit before it, I'm seeing an increase in some fuchsia ZBIs by about 13 kB.

I also tried this but am still finding some paths embedded in libunwind and libclang_rt for some reason.

In D75890#1915493, @thakis wrote:

__FILE__ only expands to an absolute path if you pass an absolute path to clang (ctrl-f "FILE" on http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html).

It feels like this is working around a deficiency in the compiler. Maybe fixing whatever's going wrong in the compiler might be better? (There's also -ffile-file-prefix map, but using that makes your _commandline_ depend on the name of the build dir, and (in clang) __FILE_NAME__ .

Hi, I believe this patch is causing the following undefined symbol error we're seeing in our 2 stage mac builders:

Submitted fixes in d2cbaf1755ffa90300365c0d71400a5ee4ada3bd

Unsure if my reply was sent over email so copying it here.

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

Hi again, I think e058667a2e017d3225a9bb067dbac7f2159576f7 might've broken our toolchain again:

Hi, this seems to have led to a test failure on our clang bots:

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

Woops, sorry. Didn't see you sent out https://reviews.llvm.org/rGe11f9fb4508534d31b09d2ba6cd22428ccc75f65.

*Bump* We're also seeing the same test fail on our aarch64 bots: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-arm64/b8887516314819686496?blamelist=1#blamelist-tab

Hi, a bisect seems to show that this causes a kernel panic for an OOM test we have for zircon (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8887545931153261696/+/steps/failures/0/steps/Linux-_2_/0/steps/attempt_0__fail_/0/steps/failed:_host_x64_oom_tests/0/logs/stdio/0).

In D71550#1880727, @ebevhan wrote:

Ah I see. Could you clarify though on how handling MIN / -EPS invokes undefined behavior?

It's the same as if we had MIN / -1 in integer arithmetic. The result is MAX+1, so we get overflow, which is UB. The same case happens for our lowering of scaled division, since if we have MIN / -EPS (-EPS is -1 in the integer representation), during widening/promotion we may potentially shift up the LHS by the amount widened, giving us MIN / -EPS again, which causes overflow when we do the integer division.

This doesn't matter for non-saturating division since the operation overflows there as well, but for saturating division we need to check the result and saturate, but the div instruction will cause an exception so we never get to that stage.

Also, if handling this UB requires an extra bit, it seems odd that we would add one a second time. I could be wrong, but isn't the reason for adding the 1st extra bit (i64 -> i65) is just to force the operands to be promoted? If so, then by the time we check LHSLead + RHSTrail < Scale + (unsigned)(Saturating && Signed)) in expandFixedPointDiv, we would've already gotten the extra bit from the initial promotion.

Yes, we add the extra bit to force promotion, but we have to shift up and consume that bit in order to preserve the saturating behavior (SelectionDAGBuilder.cpp:5477) so we don't win any leading sign bits there. We only get those when we do the promotion, but we are one bit short there because of the pre-promotion, so we end up having to widen again. expandFixedPointDiv (during promotion) and earlyExpandDIVFIX do not need to do this extra shifting since they handle saturation directly.

The premature promotion really makes fixed-point division overly complicated.

I feel like we should only need to promote once when doing fixed point division.

You are right in that the pre-promotion causes us to lose out on a bit. It would maybe be possible to skip the aforementioned one-bit shift, but then we have to tell promotion that we need one extra bit of saturation, and I don't know how to effectively propagate that information.

Hi, I think this might've caused some test failures on our bots and others on buildbot:

Hi, I think this might be triggering the test failures seen on buildbot with:

In D74704#1881012, @vvereschaka wrote:

Hello @serge-sans-paille,

looks like these changes broke the ARM builders:
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/4749
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4752

failed tests:

• FAIL: Clang::riscv64-toolchain.c
• FAIL: Clang::riscv32-toolchain.c
  
  with the following error: ` C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Driver\riscv64-toolchain.c:15:27: error: C-RV64-BAREMETAL-LP64: expected string not found in input `
  
  would you fix the problem or revert the commit?

In D73329#1873181, @ychen wrote:

In D73329#1873142, @leonardchan wrote:

Hi, a bisect seems to show that this patch resulted in a series of linker errors when building fuzzers fuchsia:

 [48364/49380] LINK user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer
 FAILED: user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map
 ../../../recipe_cleanup/clang9HU4mg/bin/clang++ -o user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug -fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -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 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy -Wl,-Map,user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.map -Wl,--start-group @'user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.rsp' -lunwind -Wl,--end-group user.vdso-x64-clang.shlib/obj/system/ulib/zircon/libzircon.so.debug user.fuzzer-x64-asan-ubsan.shlib/obj/system/ulib/c/libc.so.debug user-x64-asan-ubsan.shlib/obj/system/ulib/fdio/libfdio.so.debug && ../../../recipe_cleanup/clang9HU4mg/bin/llvm-objcopy --strip-sections "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" && case '-fuse-ld=lld -Wl,--threads -Wl,-z,combreloc -Wl,-z,relro -Wl,-z,now -Wl,-z,text -Wl,--pack-dyn-relocs=relr --target=x86_64-fuchsia -mcx16 -march=x86-64 -fcrash-diagnostics-dir=clang-crashreports -fcolor-diagnostics -Wl,--color-diagnostics -Wl,-z,max-page-size=4096 -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 -Wl,--icf=all -g3 -ffunction-sections -Wl,--gc-sections -fdata-sections -fno-exceptions -fno-rtti -nostartfiles -nolibc -Wl,-dynamic-linker=fuzzer.asan-ubsan/ld.so.1 -fsanitize=fuzzer -fsanitize=undefined -fsanitize=address -L../../zircon/public/gn/config/libc-dummy' in *build-id=none*) ;; *) ../../prebuilt/tools/buildidtool/linux-x64/buildidtool -build-id-dir ".build-id" -stamp "user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.build-id.stamp" -entry "=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer" -entry ".debug=user.fuzzer-x64-asan-ubsan/obj/system/ulib/elfload/test/elfload-fuzzer.debug" ;; esac
 ld.lld: error: undefined symbol: fuzzer::OpenProcessPipe(char const*, char const*)
>>> referenced by FuzzerDriver.cpp:321 (compiler-rt/lib/fuzzer/FuzzerDriver.cpp:321)
>>>               fuzzer.o:(fuzzer::ExecuteCommandWithPopen(fuzzer::Command const&, std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >*)) in archive ../../../recipe_cleanup/clang9HU4mg/lib/clang/11.0.0/lib/x86_64-unknown-fuchsia/libclang_rt.fuzzer.a
 
 ... and many more

Could you take a look or revert this patch? Thanks.

I tested and if there will be a revert, 4f3c3bbbf85a1283796e0e80c654779e40ce328e may also need to be reverted since it depends on this patch.

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

Apologies that I did not add OpenProcessPipe implementation on Fushia. Quick question is there popen/pclose on Fushia? Thanks.

Hi, a bisect seems to show that this patch resulted in a series of linker errors when building fuzzers fuchsia:

Actually, disregard my last comment. I think I might've made a mistake on the bisect. Sorry!

Hi, a bisect seems to show that this patche causes the build error we're seeing at https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8889138534807714304/+/steps/build/0/steps/build_fuchsia/0/steps/ninja/0/steps/zircon/0/logs/raw_io.output_failure_raw_summary_/0

In D73526#1857223, @bjope wrote:

This broke some of our downstream tests using vla:s. Those are compiled from C and executed in gdb. What happens is that DW_OP_deref disappears and then the debugger prints the wrong value.

I'll be able to take a look at this on Friday, but in the meantime if there's no fixes for this and waiting until then is too long then it's probably better to revert this. Sorry for the breakage.

*ping* Can we land this or revert D68945 in the meantime?

Ah, that's unfortunate... It's a consequence of being forced to handle the integer division overflow case gracefully.

With an i64, we get a sext+shl and an i65 after building. During the initial promotion we try to promote to i128. That gives us 63 bits of redundant sign (128-65), but in order to handle the case of MSB / ALL1 (MIN / -EPS) without invoking undefined behavior in the integer division, we require an extra bit. Since we don't have that bit to spare, we end up not being able to do the operation in i128. Then we get an i256 sdiv and there's no codegen for that.

I'm unsure what to do to make this look good. It would be convenient to skip the operation altogether in the overflow case, but we can't generate control flow here. I could check for the overflow case and fudge the inputs/result to prevent the overflow, but that feels sort of wrong. Might be the only viable option, though.

Could you also add a few test lines showing this warning doesn't appear for operations where the result is a saturated type?

It seems that I run into LLVM ERROR: Unsupported library call operation! if I call llvm.sdiv.fix.sat.i64 with a scale of 63:

Hi, it seems that this revision introduced the regression in https://bugs.llvm.org/show_bug.cgi?id=44585 that produces incorrect DWARF expressions when using safestack. Would you mind taking a look? Thanks.

LGTM with just one quick question

Hi, it seems that this commit also introduces libc++ headers that are incompatible with gcc 9.2:

LGTM for the code so far. Just want to confirm on my side that the codegen works as intended also.

Yup, was able to confirm this is the faulty patch. Would you mind looking into this or reverting in the meantime?

*Moved from the commit to here*

Hi! This seems to be causing some test failures on our bots: https://ci.chromium.org/p/fuchsia/builders/ci/clang-linux-x64/b8893521019883752048

LGTM. Could you commit this soon to get the bots green again?

Hi! I believe the commit for this (a3b2552575d3c333e928446fac10cc5b0b4092a9) is causing a test failure on our bots (https://ci.chromium.org/p/fuchsia/builders/ci/clang-linux-x64/b8894791990609720880) and some buildbots (can't find the link since it scrolled past the end of the console). Could you look into it when you get a chance?

Alrighty, it looks like everything in this patch should be ok after those 2 changes. I can send you the script I used to generate tests if you'd like to use it also.

Sorry for the holdup. Right now I'm just writing a bunch of tests to validate the IR expansion produces the correct division results. As of now, the patch LGTM, but I'd prefer holding off submitting the revision until I finish validating (unless you think it's better to submit now then fix any potential bugs that may be found later since I don't think anyone will be immediately using this). Here's a few comments I have so far:

Could you add a test to show that with a fuchsia target we end up returning this from constructors + destructors and ensure that this ABI is used?

LGTM. But just to confirm, did you test this on CQ/bots by adding ["now", "-z"] as linker flags to the config("compiler") in Fuchsia + Zircon?

Hi! I think this patch might be causing some test failures on our mac bots: