I'm wondering why the lit tests didn't catch this as part of testing rc1 on ARM. On Thu, Jan 26, 2017 at 11:25 AM, Serge Rogatch <serge.rogatch at gmail.com> wrote:> XRay is tested automatically on build-bots with tests in LLVM and > compiler-rt . Or are you asking for manual testing instructions? > Of these 2 patches, the compiler-rt patch depends on LLVM patch because the > tests compiler-rt\test\xray\TestCases\Linux would fail without the fix in > LLVM. The compiler-rt patch also enables the tests which were occasionally > disabled. > > The XRay feature is quite isolated, so side-effects are not expected. I can > only see a risk that some commit I took for granted when making&testing the > fixes, somehow didn't get to 4.0 branch. > > > On 26 January 2017 at 21:44, Hans Wennborg <hans at chromium.org> wrote: >> >> How is XRay tested? IIRC, Renato didn't see any test failures on ARM? >> >> Merging sounds reasonbaly, I'd just like to understand what's the risk >> for the branch. >> >> On Thu, Jan 26, 2017 at 10:29 AM, Serge Rogatch <serge.rogatch at gmail.com> >> wrote: >> > Hans, these changes reached trunk in https://reviews.llvm.org/rL292516 >> > and >> > https://reviews.llvm.org/rL292517 . Could you look? >> > >> > On 26 January 2017 at 03:29, Serge Rogatch <serge.rogatch at gmail.com> >> > wrote: >> >> >> >> Sorry, I initially included LLVM-Commits rather than LLVM-Dev. Fixed. >> >> >> >> On 26 January 2017 at 03:26, Serge Rogatch <serge.rogatch at gmail.com> >> >> wrote: >> >>> >> >>> Hi Dean, Renato, >> >>> >> >>> AFAIK, unfortunately, these critical Arm32 XRay fixes are not yet in >> >>> 4.0: >> >>> https://reviews.llvm.org/D28624 , https://reviews.llvm.org/D28623 . >> >>> The >> >>> first repairs XRay instrumentation map emission. Without this map XRay >> >>> doesn't work at all: the runtime doesn't see anything to instrument. >> >>> The >> >>> second fixes the CPU cache incoherency problem. Without this patch, >> >>> XRay >> >>> will intermittently fail to patch or unpatch some sleds (depending on >> >>> whether their code is in CPU cache or not). >> >>> >> >>> Is there any chance we can get these patches to 4.0? This page >> >>> http://llvm.org/docs/HowToReleaseLLVM.html#release-patch-rules says >> >>> that >> >>> "release manager, the official release testers or the code owners" may >> >>> approve cherry-picking to release branch from trunk. AFAIK, you are >> >>> code >> >>> owners for XRay and ARM. I don't know who are the release manager or >> >>> official release testers. >> >>> >> >>> Cheers, >> >>> Serge >> >> >> >> >> > > >
There were no LLVM tests for presence of XRay instrumentation map in the emitted assembly. You can see that https://reviews.llvm.org/D28624 adds this check to the tests. The tests in compiler-rt had been accidentally disabled. https://reviews.llvm.org/D28623 enables them in compiler-rt/test/xray/lit.cfg . On 26 January 2017 at 22:37, Hans Wennborg <hans at chromium.org> wrote:> I'm wondering why the lit tests didn't catch this as part of testing rc1 > on ARM. > > On Thu, Jan 26, 2017 at 11:25 AM, Serge Rogatch <serge.rogatch at gmail.com> > wrote: > > XRay is tested automatically on build-bots with tests in LLVM and > > compiler-rt . Or are you asking for manual testing instructions? > > Of these 2 patches, the compiler-rt patch depends on LLVM patch because > the > > tests compiler-rt\test\xray\TestCases\Linux would fail without the fix > in > > LLVM. The compiler-rt patch also enables the tests which were > occasionally > > disabled. > > > > The XRay feature is quite isolated, so side-effects are not expected. I > can > > only see a risk that some commit I took for granted when making&testing > the > > fixes, somehow didn't get to 4.0 branch. > > > > > > On 26 January 2017 at 21:44, Hans Wennborg <hans at chromium.org> wrote: > >> > >> How is XRay tested? IIRC, Renato didn't see any test failures on ARM? > >> > >> Merging sounds reasonbaly, I'd just like to understand what's the risk > >> for the branch. > >> > >> On Thu, Jan 26, 2017 at 10:29 AM, Serge Rogatch < > serge.rogatch at gmail.com> > >> wrote: > >> > Hans, these changes reached trunk in https://reviews.llvm.org/ > rL292516 > >> > and > >> > https://reviews.llvm.org/rL292517 . Could you look? > >> > > >> > On 26 January 2017 at 03:29, Serge Rogatch <serge.rogatch at gmail.com> > >> > wrote: > >> >> > >> >> Sorry, I initially included LLVM-Commits rather than LLVM-Dev. Fixed. > >> >> > >> >> On 26 January 2017 at 03:26, Serge Rogatch <serge.rogatch at gmail.com> > >> >> wrote: > >> >>> > >> >>> Hi Dean, Renato, > >> >>> > >> >>> AFAIK, unfortunately, these critical Arm32 XRay fixes are not yet in > >> >>> 4.0: > >> >>> https://reviews.llvm.org/D28624 , https://reviews.llvm.org/D28623 . > >> >>> The > >> >>> first repairs XRay instrumentation map emission. Without this map > XRay > >> >>> doesn't work at all: the runtime doesn't see anything to instrument. > >> >>> The > >> >>> second fixes the CPU cache incoherency problem. Without this patch, > >> >>> XRay > >> >>> will intermittently fail to patch or unpatch some sleds (depending > on > >> >>> whether their code is in CPU cache or not). > >> >>> > >> >>> Is there any chance we can get these patches to 4.0? This page > >> >>> http://llvm.org/docs/HowToReleaseLLVM.html#release-patch-rules says > >> >>> that > >> >>> "release manager, the official release testers or the code owners" > may > >> >>> approve cherry-picking to release branch from trunk. AFAIK, you are > >> >>> code > >> >>> owners for XRay and ARM. I don't know who are the release manager or > >> >>> official release testers. > >> >>> > >> >>> Cheers, > >> >>> Serge > >> >> > >> >> > >> > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170126/2404df4b/attachment.html>
I see. Thanks for clarifying. I'm Ok with merging these if Dean agrees, as I believe he's the code owner. Thanks, Hans On Thu, Jan 26, 2017 at 11:47 AM, Serge Rogatch <serge.rogatch at gmail.com> wrote:> There were no LLVM tests for presence of XRay instrumentation map in the > emitted assembly. You can see that https://reviews.llvm.org/D28624 adds this > check to the tests. > The tests in compiler-rt had been accidentally disabled. > https://reviews.llvm.org/D28623 enables them in > compiler-rt/test/xray/lit.cfg . > > On 26 January 2017 at 22:37, Hans Wennborg <hans at chromium.org> wrote: >> >> I'm wondering why the lit tests didn't catch this as part of testing rc1 >> on ARM. >> >> On Thu, Jan 26, 2017 at 11:25 AM, Serge Rogatch <serge.rogatch at gmail.com> >> wrote: >> > XRay is tested automatically on build-bots with tests in LLVM and >> > compiler-rt . Or are you asking for manual testing instructions? >> > Of these 2 patches, the compiler-rt patch depends on LLVM patch because >> > the >> > tests compiler-rt\test\xray\TestCases\Linux would fail without the fix >> > in >> > LLVM. The compiler-rt patch also enables the tests which were >> > occasionally >> > disabled. >> > >> > The XRay feature is quite isolated, so side-effects are not expected. I >> > can >> > only see a risk that some commit I took for granted when making&testing >> > the >> > fixes, somehow didn't get to 4.0 branch. >> > >> > >> > On 26 January 2017 at 21:44, Hans Wennborg <hans at chromium.org> wrote: >> >> >> >> How is XRay tested? IIRC, Renato didn't see any test failures on ARM? >> >> >> >> Merging sounds reasonbaly, I'd just like to understand what's the risk >> >> for the branch. >> >> >> >> On Thu, Jan 26, 2017 at 10:29 AM, Serge Rogatch >> >> <serge.rogatch at gmail.com> >> >> wrote: >> >> > Hans, these changes reached trunk in >> >> > https://reviews.llvm.org/rL292516 >> >> > and >> >> > https://reviews.llvm.org/rL292517 . Could you look? >> >> > >> >> > On 26 January 2017 at 03:29, Serge Rogatch <serge.rogatch at gmail.com> >> >> > wrote: >> >> >> >> >> >> Sorry, I initially included LLVM-Commits rather than LLVM-Dev. >> >> >> Fixed. >> >> >> >> >> >> On 26 January 2017 at 03:26, Serge Rogatch <serge.rogatch at gmail.com> >> >> >> wrote: >> >> >>> >> >> >>> Hi Dean, Renato, >> >> >>> >> >> >>> AFAIK, unfortunately, these critical Arm32 XRay fixes are not yet >> >> >>> in >> >> >>> 4.0: >> >> >>> https://reviews.llvm.org/D28624 , https://reviews.llvm.org/D28623 . >> >> >>> The >> >> >>> first repairs XRay instrumentation map emission. Without this map >> >> >>> XRay >> >> >>> doesn't work at all: the runtime doesn't see anything to >> >> >>> instrument. >> >> >>> The >> >> >>> second fixes the CPU cache incoherency problem. Without this patch, >> >> >>> XRay >> >> >>> will intermittently fail to patch or unpatch some sleds (depending >> >> >>> on >> >> >>> whether their code is in CPU cache or not). >> >> >>> >> >> >>> Is there any chance we can get these patches to 4.0? This page >> >> >>> http://llvm.org/docs/HowToReleaseLLVM.html#release-patch-rules says >> >> >>> that >> >> >>> "release manager, the official release testers or the code owners" >> >> >>> may >> >> >>> approve cherry-picking to release branch from trunk. AFAIK, you are >> >> >>> code >> >> >>> owners for XRay and ARM. I don't know who are the release manager >> >> >>> or >> >> >>> official release testers. >> >> >>> >> >> >>> Cheers, >> >> >>> Serge >> >> >> >> >> >> >> >> > >> > >> > > >