Page MenuHomePhabricator

Run non-filechecked commands in update_cc_test_checks.py
ClosedPublic

Authored by ggeorgakoudis on Feb 19 2021, 10:57 AM.

Details

Summary

Some tests in clang require running non-filechecked commands to generate the actual filecheck input. For example, tests for openmp offloading require generating the host bc without any checking, before running the clang command to actually generate the filechecked IR of the target device. This patch enables update_cc_test_checks.py to run non-filechecked run lines in-place.

Diff Detail

Event Timeline

ggeorgakoudis requested review of this revision.Feb 19 2021, 10:57 AM
ggeorgakoudis created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 10:57 AM
ggeorgakoudis edited the summary of this revision. (Show Details)Feb 19 2021, 11:00 AM
ggeorgakoudis added a reviewer: jdoerfert.

Update implementation to support running non-clang runlines, add tests

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2021, 11:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jdoerfert accepted this revision.Mar 8 2021, 7:11 AM

LG, thx!

This revision is now accepted and ready to land.Mar 8 2021, 7:11 AM
This revision was landed with ongoing or failed builds.Mar 8 2021, 7:18 AM
This revision was automatically updated to reflect the committed changes.

Looks like this breaks tests on mac: http://45.33.8.238/macm1/5075/step_6.txt

Please take a look, and revert for now if it takes a while to fix.

Looks like this breaks tests on mac: http://45.33.8.238/macm1/5075/step_6.txt

Please take a look, and revert for now if it takes a while to fix.

Hey @thakis, it should be an easy fix. I see no check lines are generated and I suspect something may be going wrong with the cp runline, but it's hard to know without replicating it. Is there any way you can give me temporary access to the mac machine to test things out?

Otherwise, I will simplify the test to exclude the cp runline.

IMO this should also handle the %t substitution and create a temporary directory. Otherwise we end up writing files to whatever the current cwd is.

Looks like this breaks tests on mac: http://45.33.8.238/macm1/5075/step_6.txt

Please take a look, and revert for now if it takes a while to fix.

Hey @thakis, it should be an easy fix. I see no check lines are generated and I suspect something may be going wrong with the cp runline, but it's hard to know without replicating it. Is there any way you can give me temporary access to the mac machine to test things out?

Otherwise, I will simplify the test to exclude the cp runline.

The issue has something to do with Darwin's USER_LABEL_PREFIX mangling inserting an _ prefix. This "fixes" the test, but doesn't solve whatever underlying problem is there:

diff --git a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
index bf18179392dc..a0a826c3d0b4 100644
--- a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
+++ b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c
@@ -1,7 +1,7 @@
 // Check that the non-clang/non-filechecked runlines execute
 // RUN: cp %s %s.copy.c
-// RUN: %clang_cc1 -fopenmp %s.copy.c -emit-llvm-bc -o %t-host.bc
-// RUN: %clang_cc1 -fopenmp -fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp %s.copy.c -emit-llvm-bc -o %t-host.bc
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
 
 void use(int);
 
diff --git a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
index ffc8caac0f23..3ad33fc683bd 100644
--- a/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
+++ b/clang/test/utils/update_cc_test_checks/Inputs/exec-all-runlines.c.expected
@@ -1,8 +1,8 @@
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // Check that the non-clang/non-filechecked runlines execute
 // RUN: cp %s %s.copy.c
-// RUN: %clang_cc1 -fopenmp %s.copy.c -emit-llvm-bc -o %t-host.bc
-// RUN: %clang_cc1 -fopenmp -fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp %s.copy.c -emit-llvm-bc -o %t-host.bc
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-host-ir-file-path %t-host.bc %s.copy.c -emit-llvm -o - | FileCheck %s
 
 void use(int);

You should be able to replicate the failure locally by setting an apple triple instead, i.e. -triple x86_64-apple-macosx.

jroelofs reopened this revision.Mar 8 2021, 5:30 PM

Reverted in a24644bb1ce09b40c2d751569dd5bb37ea9c995d to get the bots green again. Feel free to re-land with a fix.

This revision is now accepted and ready to land.Mar 8 2021, 5:30 PM

@jroelofs, thank you very much for the much useful information. I will try to replicate the problem with the apple triple and fix it, if possible, or otherwise use your patch to avoid the issue.

Add triple to avoid spurious failures in tests

This revision was landed with ongoing or failed builds.Mar 10 2021, 12:25 PM
This revision was automatically updated to reflect the committed changes.

Add triple to avoid spurious failures in tests

They're not really spurious, right... Doesn't this mean that utils/update_cc_test_checks.py is now broken for people who develop on a mac?

Add triple to avoid spurious failures in tests

They're not really spurious, right... Doesn't this mean that utils/update_cc_test_checks.py is now broken for people who develop on a mac?

It's spurious in the sense that it's not this commit that breaks mac tests. Testing on mac is already broken. By investigating the issue, you are right that the USER_LABEL_PREFIX _ is skipping generating the checks because function names mismatch in IR output. I suppose the issue should be fixed by identifying a mac triple and removing the _ prefix?