Merge branch 'main' into wait_reference

Author: StarOne01

.ci/generate_test_report.py

@@ -19,12 +19,13 @@ def junit_from_xml(xml):
 
 class TestReports(unittest.TestCase):
     def test_title_only(self):
-        self.assertEqual(_generate_report("Foo", []), ("", "success"))
+        self.assertEqual(_generate_report("Foo", 0, []), ("", "success"))
 
     def test_no_tests_in_testsuite(self):
         self.assertEqual(
             _generate_report(
                 "Foo",
+                1,
                 [
                     junit_from_xml(
                         dedent(
@@ -45,6 +46,7 @@ def test_no_failures(self):
         self.assertEqual(
             _generate_report(
                 "Foo",
+                0,
                 [
                     junit_from_xml(
                         dedent(
@@ -70,10 +72,51 @@ def test_no_failures(self):
             ),
         )
 
+    def test_no_failures_build_failed(self):
+        self.assertEqual(
+            _generate_report(
+                "Foo",
+                1,
+                [
+                    junit_from_xml(
+                        dedent(
+                            """\
+          <?xml version="1.0" encoding="UTF-8"?>
+          <testsuites time="0.00">
+          <testsuite name="Passed" tests="1" failures="0" skipped="0" time="0.00">
+          <testcase classname="Bar/test_1" name="test_1" time="0.00"/>
+          </testsuite>
+          </testsuites>"""
+                        )
+                    )
+                ],
+                buildkite_info={
+                    "BUILDKITE_ORGANIZATION_SLUG": "organization_slug",
+                    "BUILDKITE_PIPELINE_SLUG": "pipeline_slug",
+                    "BUILDKITE_BUILD_NUMBER": "build_number",
+                    "BUILDKITE_JOB_ID": "job_id",
+                },
+            ),
+            (
+                dedent(
+                    """\
+              # Foo
+
+              * 1 test passed
+
+              All tests passed but another part of the build **failed**.
+
+              [Download](https://buildkite.com/organizations/organization_slug/pipelines/pipeline_slug/builds/build_number/jobs/job_id/download.txt) the build's log file to see the details."""
+                ),
+                "error",
+            ),
+        )
+
     def test_report_single_file_single_testsuite(self):
         self.assertEqual(
             _generate_report(
                 "Foo",
+                1,
                 [
                     junit_from_xml(
                         dedent(
@@ -166,6 +209,7 @@ def test_report_single_file_multiple_testsuites(self):
         self.assertEqual(
             _generate_report(
                 "ABC and DEF",
+                1,
                 [
                     junit_from_xml(
                         dedent(
@@ -198,6 +242,7 @@ def test_report_multiple_files_multiple_testsuites(self):
         self.assertEqual(
             _generate_report(
                 "ABC and DEF",
+                1,
                 [
                     junit_from_xml(
                         dedent(
@@ -238,6 +283,7 @@ def test_report_dont_list_failures(self):
         self.assertEqual(
             _generate_report(
                 "Foo",
+                1,
                 [
                     junit_from_xml(
                         dedent(
@@ -272,6 +318,7 @@ def test_report_dont_list_failures_link_to_log(self):
         self.assertEqual(
             _generate_report(
                 "Foo",
+                1,
                 [
                     junit_from_xml(
                         dedent(
@@ -312,6 +359,7 @@ def test_report_size_limit(self):
         self.assertEqual(
             _generate_report(
                 "Foo",
+                1,
                 [
                     junit_from_xml(
                         dedent(
@@ -351,12 +399,18 @@ def test_report_size_limit(self):
 # and output will not be.
 def _generate_report(
     title,
+    return_code,
     junit_objects,
     size_limit=1024 * 1024,
     list_failures=True,
     buildkite_info=None,
 ):
     if not junit_objects:
+        # Note that we do not post an empty report, therefore we can ignore a
+        # non-zero return code in situations like this.
+        #
+        # If we were going to post a report, then yes, it would be misleading
+        # to say we succeeded when the final return code was non-zero.
         return ("", "success")
 
     failures = {}
@@ -385,7 +439,11 @@ def _generate_report(
     if not tests_run:
         return ("", None)
 
-    style = "error" if tests_failed else "success"
+    style = "success"
+    # Either tests failed, or all tests passed but something failed to build.
+    if tests_failed or return_code != 0:
+        style = "error"
+
     report = [f"# {title}", ""]
 
     tests_passed = tests_run - tests_skipped - tests_failed
@@ -400,17 +458,17 @@ def plural(num_tests):
     if tests_failed:
         report.append(f"* {tests_failed} {plural(tests_failed)} failed")
 
-    if not list_failures:
-        if buildkite_info is not None:
-            log_url = (
-                "https://buildkite.com/organizations/{BUILDKITE_ORGANIZATION_SLUG}/"
-                "pipelines/{BUILDKITE_PIPELINE_SLUG}/builds/{BUILDKITE_BUILD_NUMBER}/"
-                "jobs/{BUILDKITE_JOB_ID}/download.txt".format(**buildkite_info)
-            )
-            download_text = f"[Download]({log_url})"
-        else:
-            download_text = "Download"
+    if buildkite_info is not None:
+        log_url = (
+            "https://buildkite.com/organizations/{BUILDKITE_ORGANIZATION_SLUG}/"
+            "pipelines/{BUILDKITE_PIPELINE_SLUG}/builds/{BUILDKITE_BUILD_NUMBER}/"
+            "jobs/{BUILDKITE_JOB_ID}/download.txt".format(**buildkite_info)
+        )
+        download_text = f"[Download]({log_url})"
+    else:
+        download_text = "Download"
 
+    if not list_failures:
         report.extend(
             [
                 "",
@@ -435,11 +493,23 @@ def plural(num_tests):
                         "</details>",
                     ]
                 )
+    elif return_code != 0:
+        # No tests failed but the build was in a failed state. Bring this to the user's
+        # attention.
+        report.extend(
+            [
+                "",
+                "All tests passed but another part of the build **failed**.",
+                "",
+                f"{download_text} the build's log file to see the details.",
+            ]
+        )
 
     report = "\n".join(report)
     if len(report.encode("utf-8")) > size_limit:
         return _generate_report(
             title,
+            return_code,
             junit_objects,
             size_limit,
             list_failures=False,
@@ -449,9 +519,10 @@ def plural(num_tests):
     return report, style
 
 
-def generate_report(title, junit_files, buildkite_info):
+def generate_report(title, return_code, junit_files, buildkite_info):
     return _generate_report(
         title,
+        return_code,
         [JUnitXml.fromfile(p) for p in junit_files],
         buildkite_info=buildkite_info,
     )
@@ -463,6 +534,7 @@ def generate_report(title, junit_files, buildkite_info):
         "title", help="Title of the test report, without Markdown formatting."
     )
     parser.add_argument("context", help="Annotation context to write to.")
+    parser.add_argument("return_code", help="The build's return code.", type=int)
     parser.add_argument("junit_files", help="Paths to JUnit report files.", nargs="*")
     args = parser.parse_args()
 
@@ -477,7 +549,9 @@ def generate_report(title, junit_files, buildkite_info):
     if len(buildkite_info) != len(env_var_names):
         buildkite_info = None
 
-    report, style = generate_report(args.title, args.junit_files, buildkite_info)
+    report, style = generate_report(
+        args.title, args.return_code, args.junit_files, buildkite_info
+    )
 
     if report:
         p = subprocess.Popen(

.ci/metrics/metrics.py

@@ -12,7 +12,7 @@
     "https://influx-prod-13-prod-us-east-0.grafana.net/api/v1/push/influx/write"
 )
 GITHUB_PROJECT = "llvm/llvm-project"
-WORKFLOWS_TO_TRACK = ["Check code formatting", "LLVM Premerge Checks"]
+WORKFLOWS_TO_TRACK = ["LLVM Premerge Checks"]
 SCRAPE_INTERVAL_SECONDS = 5 * 60
 
 
@@ -80,6 +80,18 @@ def get_metrics(github_repo: github.Repository, workflows_to_track: dict[str, in
         completed_at = workflow_jobs[0].completed_at
 
         job_result = int(workflow_jobs[0].conclusion == "success")
+        if job_result:
+            # We still might want to mark the job as a failure if one of the steps
+            # failed. This is required due to use setting continue-on-error in
+            # the premerge pipeline to prevent sending emails while we are
+            # testing the infrastructure.
+            # TODO(boomanaiden154): Remove this once the premerge pipeline is no
+            # longer in a testing state and we can directly assert the workflow
+            # result.
+            for step in workflow_jobs[0].steps:
+                if step.conclusion != "success":
+                    job_result = 0
+                    break
 
         queue_time = started_at - created_at
         run_time = completed_at - started_at

.ci/monolithic-linux.sh

@@ -29,6 +29,8 @@ if [[ -n "${CLEAR_CACHE:-}" ]]; then
 fi
 
 function at-exit {
+  retcode=$?
+
   mkdir -p artifacts
   ccache --print-stats > artifacts/ccache_stats.txt
 
@@ -37,7 +39,7 @@ function at-exit {
   if command -v buildkite-agent 2>&1 >/dev/null
   then
     python3 "${MONOREPO_ROOT}"/.ci/generate_test_report.py ":linux: Linux x64 Test Results" \
-      "linux-x64-test-results" "${BUILD_DIR}"/test-results.*.xml
+      "linux-x64-test-results" $retcode "${BUILD_DIR}"/test-results.*.xml
   fi
 }
 trap at-exit EXIT

.ci/monolithic-windows.sh

@@ -28,6 +28,8 @@ fi
 
 sccache --zero-stats
 function at-exit {
+  retcode=$?
+
   mkdir -p artifacts
   sccache --show-stats >> artifacts/sccache_stats.txt
 
@@ -36,7 +38,7 @@ function at-exit {
   if command -v buildkite-agent 2>&1 >/dev/null
   then
     python "${MONOREPO_ROOT}"/.ci/generate_test_report.py ":windows: Windows x64 Test Results" \
-      "windows-x64-test-results" "${BUILD_DIR}"/test-results.*.xml
+      "windows-x64-test-results" $retcode "${BUILD_DIR}"/test-results.*.xml
   fi
 }
 trap at-exit EXIT

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -33,6 +33,7 @@
 #include "InaccurateEraseCheck.h"
 #include "IncDecInConditionsCheck.h"
 #include "IncorrectEnableIfCheck.h"
+#include "IncorrectEnableSharedFromThisCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
@@ -144,6 +145,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectEnableIfCheck>(
         "bugprone-incorrect-enable-if");
+    CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>(
+        "bugprone-incorrect-enable-shared-from-this");
     CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>(
         "bugprone-return-const-ref-from-parameter");
     CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -27,6 +27,11 @@ add_clang_library(clangTidyBugproneModule STATIC
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
+  IncorrectEnableIfCheck.cpp
+  IncorrectEnableSharedFromThisCheck.cpp
+  ReturnConstRefFromParameterCheck.cpp
+  SuspiciousStringviewDataUsageCheck.cpp
+  SwitchMissingDefaultCaseCheck.cpp
   IncDecInConditionsCheck.cpp
   IncorrectEnableIfCheck.cpp
   IncorrectRoundingsCheck.cpp

clang-tools-extra/clang-tidy/bugprone/IncorrectEnableSharedFromThisCheck.cpp

@@ -0,0 +1,65 @@
+//===--- IncorrectEnableSharedFromThisCheck.cpp - clang-tidy --------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "IncorrectEnableSharedFromThisCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void IncorrectEnableSharedFromThisCheck::registerMatchers(MatchFinder *Finder) {
+  const auto EnableSharedFromThis =
+      cxxRecordDecl(hasName("enable_shared_from_this"), isInStdNamespace());
+  const auto QType = hasCanonicalType(hasDeclaration(
+      cxxRecordDecl(
+          anyOf(EnableSharedFromThis.bind("enable_rec"),
+                cxxRecordDecl(hasAnyBase(cxxBaseSpecifier(
+                    isPublic(), hasType(hasCanonicalType(
+                                    hasDeclaration(EnableSharedFromThis))))))))
+          .bind("base_rec")));
+  Finder->addMatcher(
+      cxxRecordDecl(
+          unless(isExpansionInSystemHeader()),
+          hasDirectBase(cxxBaseSpecifier(unless(isPublic()), hasType(QType))
+                            .bind("base")))
+          .bind("derived"),
+      this);
+}
+
+void IncorrectEnableSharedFromThisCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *BaseSpec = Result.Nodes.getNodeAs<CXXBaseSpecifier>("base");
+  const auto *Base = Result.Nodes.getNodeAs<CXXRecordDecl>("base_rec");
+  const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+  const bool IsEnableSharedFromThisDirectBase =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("enable_rec") == Base;
+  const bool HasWrittenAccessSpecifier =
+      BaseSpec->getAccessSpecifierAsWritten() != AS_none;
+  const auto ReplacementRange = CharSourceRange(
+      SourceRange(BaseSpec->getBeginLoc()), HasWrittenAccessSpecifier);
+  const llvm::StringRef Replacement =
+      HasWrittenAccessSpecifier ? "public" : "public ";
+  const FixItHint Hint =
+      IsEnableSharedFromThisDirectBase
+          ? FixItHint::CreateReplacement(ReplacementRange, Replacement)
+          : FixItHint();
+  diag(Derived->getLocation(),
+       "%2 is not publicly inheriting from "
+       "%select{%1 which inherits from |}0'std::enable_shared_"
+       "from_this', "
+       "which will cause unintended behaviour "
+       "when using 'shared_from_this'; make the inheritance "
+       "public",
+       DiagnosticIDs::Warning)
+      << IsEnableSharedFromThisDirectBase << Base << Derived << Hint;
+}
+
+} // namespace clang::tidy::bugprone