Page MenuHomePhabricator

[Clang][Misc] Fix fragile test
ClosedPublic

Authored by atmnpatel on Dec 30 2020, 7:25 PM.

Details

Summary

This test has %clang in the run line when it should have %clang_cc1. When built in release mode, %clang will discard labels for blocks, so it is unable to check for block labels.

Diff Detail

Event Timeline

atmnpatel created this revision.Dec 30 2020, 7:25 PM
atmnpatel requested review of this revision.Dec 30 2020, 7:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2020, 7:25 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Do you know why some bots don't emit entry:?

I can't say that I know with any certainty, I'm too inexperienced. This failure was missed by me locally, the pre-merge bot, and by most of the buildbots other than the ones I mentioned above. I have no idea under what configuration of CMake options will clang not emit this particular case of a label for a BB.

MaskRay added a comment.EditedDec 30 2020, 8:01 PM

You can check the CMake configurations of these bots.

https://github.com/llvm/llvm-zorg buildbot/osuosl/master/config/builders.py

I think it'd be good to know why. It may be a bug.

fhahn added a subscriber: fhahn.Dec 31 2020, 4:24 AM

I can't say that I know with any certainty, I'm too inexperienced. This failure was missed by me locally, the pre-merge bot, and by most of the buildbots other than the ones I mentioned above. I have no idea under what configuration of CMake options will clang not emit this particular case of a label for a BB.

The problem is that this test is using %clang instead of %clang_cc1. Executing clang directly will pass -fdiscard-value-names when built with release mode; all blocks/values will be numbered, instead of explicitly named. It would be better to update the test to use %clang_cc1, to make it independent on flags passed depending on configuration of clang.

Ah I see, I was never able to find clear documentation on what exactly the -cc1 flag does other than the conceptual description. This should fix it right?

Ah I see, I was never able to find clear documentation on what exactly the -cc1 flag does other than the conceptual description. This should fix it right?

-cc1 options are implementation details and are unstable. Driver options need to be more stable and usually cannot be removed.

Ah I forgot that the driver option's default -f[no-]discard-value-names is dependent on -DLLVM_ENABLE_ASSERTIONS.

Can you also update the summary of this Diff and add a file-level comment to the test?

atmnpatel edited the summary of this revision. (Show Details)Dec 31 2020, 10:34 AM
xbolva00 accepted this revision.Dec 31 2020, 10:46 AM
This revision is now accepted and ready to land.Dec 31 2020, 10:46 AM
This revision was landed with ongoing or failed builds.Dec 31 2020, 10:49 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Dec 31 2020, 11:19 AM
clang/test/Misc/loop-opt-setup.c
1

Most tests use %clang_cc1 for relatively stable flag behavior so this comment is not needed.

The comment should mention the purpose of this test and probably why -O1 is used.