Selector unification: use == for comparison #2738
Conversation
|
This needs to be rebased on master. |
Extracted from sass#2315
|
Done |
|
I'm heading to bed for the night. Looks like a previous change is causing
CI to fail after landing of master. It's causing all subsequent PRs to
fail. I'll try to investigate further tomorrow unless you beat me to it.
…On Mon., 19 Nov. 2018, 9:28 pm Gleb Mazovetskiy ***@***.*** wrote:
Done
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2738 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAjZWAJK8OeH3mYBD2ZdjQo9dWUq2o9dks5uwoe8gaJpZM4Yn-Mf>
.
|
|
@xzyfer The first failing commit seems to be the one that disables coverage for clang: d09d09d It looks like libsass may have been leaking for a while but this went unnoticed because ASAN is disabled when coverage is enabled: libsass/script/ci-build-libsass Lines 57 to 59 in 3e60b12 |
|
Yes however that change is intended. This suggests something troublesome
was introduced in the small window where the configuration was incorrect,
between d09d09d and ab03dc0. The obvious candidate appears to be d225a09.
…On Tue., 20 Nov. 2018, 5:40 am Gleb Mazovetskiy ***@***.*** wrote:
@xzyfer <https://github.com/xzyfer> The first failing commit seems to be
this one:
d09d09d
<d09d09d>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2738 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAjZWDkpFLryOmHgRoYfHFCH2V7ml8Tuks5uwvsCgaJpZM4Yn-Mf>
.
|
|
This is what I use for investigating by the way: enable_asan() {
export EXTRA_CXXFLAGS='-fsanitize=address -g -O0 -fno-omit-frame-pointer'
export EXTRA_CFLAGS="$EXTRA_CXXFLAGS"
export EXTRA_LDFLAGS="-fsanitize=address -lstdc++"
export ASAN_OPTIONS="verbosity=1:symbolize=1"
export CC=clang
export CXX=clang++
}
enable_asan
cd sassc && make clean && cd - && make clean && make -j8 test |
Maybe not that small, it fails on d09d09d as well. |
|
Bisecting (manually, running on the current test suite but only considering ASAN errors):
|
|
According to the bisection results, the first ASAN failure is at 205dc65 |
Media_Blocks are prone to have circular references. Copy could leak memory if it does not get picked up. Looks like we are able to reset block reference for copy Good as it will ensure a low memory overhead for this fix So this is a cheap solution with a minimal price
Aha apologies, I missed this comment. |
|
My PRs each address individual leaks. With all 3 there are no more leaks. |
|
I don't get notified of edits via email. It's easier for me to respond in a timely manner if you can avoid editing comments :) |
|
OK, will do so in the future. CI was only passing because it wasn't running ASAN (until you disabled coverage). Memory leaks existed since March 17. |
|
Thanks for looking into this. I agree it is weird. I'm catching up on the backlog before going to bed. |
|
I believe I've merged all the leak fix PRs however CI still appears to be unhappy - https://travis-ci.org/sass/libsass/jobs/457400390 I wonder if there is a config issue for that build. |
|
Yes, there must be, because the error message in that build is: |
|
Looks like it doesn't work in a docker container without Looks like there is no way to add container capabilities on Travis. However, we can simply set |
Extracted from #2315
@mgreter