[llvm] b01f499 - [NFC][Verifier] Split token1.ll into two, assert/non-assert versions

[David Blaikie via llvm-commits]

On Wed, May 5, 2021 at 6:39 AM Roman Lebedev <lebedev.ri at gmail.com> wrote:
>
> On Tue, May 4, 2021 at 2:08 AM David Blaikie <dblaikie at gmail.com> wrote:
> >
> > On Mon, May 3, 2021 at 12:42 AM Nikita Popov <nikita.ppv at gmail.com> wrote:
> > >
> > > On Mon, May 3, 2021 at 8:40 AM Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> > >>
> > >> Doesn't verifier run on the fully constructed IR?
> > >> I.e. won't it always run after said constructor has run?
> > >> I'm not really sure how we can perform that verifier check earlier?
> > >>
> > >> Roman
> > >
> > >
> > > I think you need to either drop the assert, or check this condition already in LLParser, maybe somewhere around https://github.com/llvm/llvm-project/blob/1f8963c80195aa86d02e81acedcf1ff3da127842/llvm/lib/AsmParser/LLParser.cpp#L7175. And possibly the Bitcode parser as well?
> >
> > Sounds right/like the right direction to me.
> My point being, iff we do that *instead* of verifier check,
> then we will no longer have a safety net for when the LLVM
> is compiled without assertions, and some transform creates such a PHI.

Yep, generally there is no safety net for violating the contract (ie:
doing something that would cause an assert) in a non-asserts build
(like buffer overruns, etc - violate the assertion/constraint and you
get UB). That's acceptable/expected as far as I'm concerned.

- Dave

> So i'm still a little bit confused.
>
> Roman
>
> > >
> > > Nikita
> > >
> > >>
> > >> On Sun, May 2, 2021 at 10:46 PM David Blaikie <dblaikie at gmail.com> wrote:
> > >> >
> > >> > On Sun, May 2, 2021 at 12:34 PM Roman Lebedev <lebedev.ri at gmail.com> wrote:
> > >> >>
> > >> >> Originally we had a verifier check for phis with token type,
> > >> >> i made the PHINode constructor assert if the type is a token,
> > >> >> which obviously broke the verifier test.
> > >> >
> > >> >
> > >> > Sounds like the verifier may need to be changed to check this condition earlier, then? We shouldn't be able to reach/fire an assertion on any input when running the verifier, I think.
> > >> >
> > >> >>
> > >> >> If we drop the test for what happens without assertions,
> > >> >> that implies we no longer care what the verifier does
> > >> >> about such PHI's. Is that really so?
> > >> >>
> > >> >> Roman
> > >> >>
> > >> >> On Sun, May 2, 2021 at 10:11 PM David Blaikie <dblaikie at gmail.com> wrote:
> > >> >> >
> > >> >> > Could you explain the purpose of these two tests a bit more?
> > >> >> >
> > >> >> > In general I don't think we should be testing the behavior of assertions except in pretty narrow cases (like checking that the Error API asserts in certain ways that are critical to its usability) - since they represent essentially "undefined behavior".
> > >> >> >
> > >> >> > And if we are testing the behavior of an assertion, we shouldn't be testing what happens without assertions - because, again, if an assertion can be triggered then the behavior "behind" the assert is undefined, we shouldn't expect any particular behavior. (if we expect particular behavior, we shouldn't assert)
> > >> >> >
> > >> >> > On Wed, Apr 28, 2021 at 3:59 AM Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> > >> >> >>
> > >> >> >>
> > >> >> >> Author: Roman Lebedev
> > >> >> >> Date: 2021-04-28T13:58:38+03:00
> > >> >> >> New Revision: b01f499861235f414f6740bbf950d095bceada4d
> > >> >> >>
> > >> >> >> URL: https://github.com/llvm/llvm-project/commit/b01f499861235f414f6740bbf950d095bceada4d
> > >> >> >> DIFF: https://github.com/llvm/llvm-project/commit/b01f499861235f414f6740bbf950d095bceada4d.diff
> > >> >> >>
> > >> >> >> LOG: [NFC][Verifier] Split token1.ll into two, assert/non-assert versions
> > >> >> >>
> > >> >> >> Added:
> > >> >> >>     llvm/test/Verifier/token1-with-asserts.ll
> > >> >> >>     llvm/test/Verifier/token1-without-asserts.ll
> > >> >> >>
> > >> >> >> Modified:
> > >> >> >>
> > >> >> >>
> > >> >> >> Removed:
> > >> >> >>     llvm/test/Verifier/token1.ll
> > >> >> >>
> > >> >> >>
> > >> >> >> ################################################################################
> > >> >> >> diff  --git a/llvm/test/Verifier/token1.ll b/llvm/test/Verifier/token1-with-asserts.ll
> > >> >> >> similarity index 100%
> > >> >> >> rename from llvm/test/Verifier/token1.ll
> > >> >> >> rename to llvm/test/Verifier/token1-with-asserts.ll
> > >> >> >>
> > >> >> >> diff  --git a/llvm/test/Verifier/token1-without-asserts.ll b/llvm/test/Verifier/token1-without-asserts.ll
> > >> >> >> new file mode 100644
> > >> >> >> index 000000000000..ef2d25c045d2
> > >> >> >> --- /dev/null
> > >> >> >> +++ b/llvm/test/Verifier/token1-without-asserts.ll
> > >> >> >> @@ -0,0 +1,12 @@
> > >> >> >> +; REQUIRES: !asserts
> > >> >> >> +; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
> > >> >> >> +
> > >> >> >> +define void @f(token %A, token %B) {
> > >> >> >> +entry:
> > >> >> >> +  br label %bb
> > >> >> >> +
> > >> >> >> +bb:
> > >> >> >> +  %phi = phi token [ %A, %bb ], [ %B, %entry]
> > >> >> >> +; CHECK: PHI nodes cannot have token type!
> > >> >> >> +  br label %bb
> > >> >> >> +}
> > >> >> >>
> > >> >> >>
> > >> >> >>
> > >> >> >> _______________________________________________
> > >> >> >> llvm-commits mailing list
> > >> >> >> llvm-commits at lists.llvm.org
> > >> >> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > >> _______________________________________________
> > >> llvm-commits mailing list
> > >> llvm-commits at lists.llvm.org
> > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits