On armv7 host, using -fsanitize=bounds causes Clang to crash

[Quuxplusone]

Bugzilla Link	PR46117
Status	NEW
Importance	P normal
Reported by	Marco Elver (elver@google.com)
Reported on	2020-05-28 01:48:09 -0700
Last modified on	2021-06-09 15:13:55 -0700
Version	trunk
Hardware	Other All
CC	aaronpuchert@alice-dsl.net, douglas_yung@playstation.sony.com, htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org, luismarques@lowrisc.org, neeilans@live.com, nigelp@xmos.com, richard-llvm@metafoo.co.uk, tstellar@redhat.com, zixuan.wu@linux.alibaba.com
Fixed by commit(s)
Attachments	linux-armv7.log (4402 bytes, text/x-log)
Blocks	PR49317
Blocked by
See also

Created attachment 23546
linux armv7 log

Using -fsanitize=bounds when LLVM has been compiled for the armv7 or thumbv7
architectures appears to be unsupported.

See the attached log for a failure.

[Quuxplusone]

Attached linux-armv7.log (4402 bytes, text/x-log): linux armv7 log

[Quuxplusone]

Hmm, that test isn't failing though when assertions are disabled (at least for
me on armv7l, also armv6l):

1 warning(s) in tests
********************
Unexpectedly Passed Tests (1):
  Clang :: CodeGen/sanitize-coverage.c

Is there a way we could mark this as XFAIL only for builds with assertions?

Interestingly though the test fails for me on s390x even without assertions.

[Quuxplusone]

I should perhaps mention that this is on 11.0.0rc1, not on master, but I think the bug was opened before the branch point.

[Quuxplusone]

> Is there a way we could mark this as XFAIL only for builds with assertions?
>
> Interestingly though the test fails for me on s390x even without assertions.

That's the wrong way around.

If there is an assertion that fails, that really indicates some underlying
problem that hasn't been addressed properly. And if used with assertions off,
there is no telling what strange behaviour you might get even though it doesn't
fail loudly.

If the assertion is incorrect, the assertion should be fixed.

Otherwise, the release build should be changed to also emit an error if used on
an unsupported architecture.

[Quuxplusone]

I'm not saying there isn't a problem, I'm just saying that the XFAIL is an issue for people who build without assertions. If there is an XFAIL the test is treated as failure if it doesn't fail. And this test doesn't reliably fail.

[Quuxplusone]

We should probably just skip the test if it runs into assertions, as you said, there is no telling what strange behavior you might get. XFAIL should only be used for tests that reliably fail, for example because the results are wrong.

[Quuxplusone]

With the update to use the new pass manager by default, this test is now XPassing.

http://lab.llvm.org:8011/#/builders/26/builds/1271

Is further investigation needed with the old pass manager since we are now defaulting to the new one? If not, can we just remove the XFAIL?

[Quuxplusone]

I've copied the test sanitize-coverage.c to sanitize-coverage-old-pm.c which forces the old pass manager to be used and migrated over the XFAIL to the new test in commit 7b4832648a6339c798f1f72bbc88b1ee41e9a338.

This will fix an XPASS that was happening because the test no longer fails with the new pass manager.

[Quuxplusone]

The same issue happens on a riscv32 host. I'll add an XFAIL for that.
I suppose there might not be much benefit in investigating this, with the
increasing adoption of the new pass manager?

[Quuxplusone]

(In reply to Luís Marques from comment #8)
> The same issue happens on a riscv32 host. I'll add an XFAIL for that.
Probably better to skip the test, an assertion can't be expected to be hit.
(Builds without assertions should also pass the test suite.)

[Quuxplusone]

(In reply to Aaron Puchert from comment #9)
> (In reply to Luís Marques from comment #8)
> > The same issue happens on a riscv32 host. I'll add an XFAIL for that.
> Probably better to skip the test, an assertion can't be expected to be hit.
> (Builds without assertions should also pass the test suite.)

I meant adding an XFAIL to the new test, `sanitize-coverage-old-pm.c`. That
makes sense, no? It's exactly the same situation as {arm|thumb}v7.

[Quuxplusone]

(In reply to Luís Marques from comment #10)
> I meant adding an XFAIL to the new test, `sanitize-coverage-old-pm.c`. That
> makes sense, no? It's exactly the same situation as {arm|thumb}v7.
It's the same situation, but it should be SKIP instead of XFAIL for them as
well. Otherwise those building without assertions might get "Unexpectedly
Passed Tests" which are treated as failures. (Assuming you are observing an
assertion failure.)

Which is bad for people doing distribution builds (which usually don't have
assertions): they might think something is wrong with how they are building
LLVM.

[Quuxplusone]

(In reply to Aaron Puchert from comment #11)
> It's the same situation, but it should be SKIP instead of XFAIL for them as
> well. Otherwise those building without assertions might get "Unexpectedly
> Passed Tests" which are treated as failures. (Assuming you are observing an
> assertion failure.)

This issue occurs even in release builds. There is a (non-assert) plain if
statement to check for that condition. Otherwise, I would agree with you. In
any case, I appreciate the feedback, as it's certainly a mistake one can make.
So, presumably I can go ahead with my patch (D99108) to add the XFAIL? Notice
there already is an XFAIL for ARM.

[Quuxplusone]

(In reply to Luís Marques from comment #12)
> This issue occurs even in release builds. There is a (non-assert) plain if
> statement to check for that condition. Otherwise, I would agree with you. In
> any case, I appreciate the feedback, as it's certainly a mistake one can
> make. So, presumably I can go ahead with my patch (D99108) to add the XFAIL?
> Notice there already is an XFAIL for ARM.

Fair enough. I don't know the situation in the current tree, but it seems to
not fail in release builds of the latest release candidate 12.0.0rc3. I just
wanted to make sure that it's not just an assertion failure.

By the way, does anyone have an opinion on whether to close this bug? Do we
care about the legacy pass manager enough to fix it?

[Quuxplusone]

Also encounter in CSKY backend.

How about remove the testcase sanitize-coverage-old-pm.c?

[Quuxplusone]

What's the status of this bug? Is there a fix?

[Quuxplusone]

(In reply to Tom Stellard from comment #15)
> What's the status of this bug?  Is there a fix?
According to comment 6, this is no longer an issue with the new pass manager,
so likely nobody wants to invest the time to fix it for the old one.

I think there is nothing to port back here, but if somebody else knows better
feel free to correct me.