- User Since
- Apr 25 2018, 1:47 PM (100 w, 6 d)
Thu, Mar 26
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!
Wed, Mar 25
Hi. I think this patch might be leading to the linker error we're seeing:
Mon, Mar 23
Fri, Mar 20
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:
Thu, Mar 19
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.
Wed, Mar 18
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.
Tue, Mar 17
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.
Mon, Mar 16
Wed, Mar 11
I also tried this but am still finding some paths embedded in libunwind and libclang_rt for some reason.
Tue, Mar 10
Mon, Mar 9
Feb 26 2020
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.
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:
Feb 25 2020
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
Feb 24 2020
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).
Feb 20 2020
Hi, I think this might've caused some test failures on our bots and others on buildbot:
Feb 19 2020
Hi, I think this might be triggering the test failures seen on buildbot with:
Feb 18 2020
Feb 12 2020
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!
Feb 10 2020
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
Feb 4 2020
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.
Jan 31 2020
Jan 30 2020
*ping* Can we land this or revert D68945 in the meantime?
Jan 29 2020
Jan 28 2020
Jan 27 2020
Jan 23 2020
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.
Jan 22 2020
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:
Jan 17 2020
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.
Jan 13 2020
LGTM with just one quick question
Jan 10 2020
Hi, it seems that this commit also introduces libc++ headers that are incompatible with gcc 9.2:
Jan 9 2020
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*
Dec 20 2019
Hi! This seems to be causing some test failures on our bots: https://ci.chromium.org/p/fuchsia/builders/ci/clang-linux-x64/b8893521019883752048
Dec 6 2019
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.
Dec 4 2019
Nov 27 2019
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:
Nov 26 2019
Nov 25 2019
Nov 21 2019
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?
Nov 20 2019
Hi! I think this patch might be causing some test failures on our mac bots: