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

[melver]

Bugzilla Link	46117
Version	trunk
OS	All
Blocks	#48661
Attachments	linux armv7 log
CC	@aaronpuchert,@dyung,@luismarques,@nigelp-xmos,@zygoloid,@tstellar

Extended Description

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.

[aaronpuchert]

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.

[aaronpuchert]

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.

[melver]

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.

[aaronpuchert]

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.

[aaronpuchert]

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.

[dyung]

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?

[dyung]

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 7b48326.

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

[luismarques]

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?

[aaronpuchert]

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.)

[luismarques]

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.

[aaronpuchert]

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.

[luismarques]

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.

[aaronpuchert]

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?

[llvmbot]

Also encounter in CSKY backend.

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

[tstellar]

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