MaskRay added inline comments.
================ Comment at: clang/test/CodeGen/embed-fat-lto-objects.c:3 +// RUN: %clang -cc1 -triple x86_64-unknown-linux-gnu -flto=full -ffat-lto-objects -emit-llvm < %s | FileCheck %s --check-prefixes=FULL,SPLIT + +// RUN: %clang -cc1 -triple x86_64-unknown-linux-gnu -flto=thin -fsplit-lto-unit -ffat-lto-objects -emit-llvm < %s | FileCheck %s --check-prefixes=THIN,SPLIT ---------------- paulkirth wrote: > MaskRay wrote: > > We need a `-emit-obj` test with `// REQUIRES: x86-registered-target`. > > Use llvm-readelf to check that .llvm.lto section is present. > > > > We also need a `-S` test, otherwise the behavior of driver `clang -S > > -ffat-lto-objects` is untested. > > > > > Sure, I can do both of those, but as a general question, isn't the generation > of assembly the responsibility of the backend? I believe we have some tests > that use `llc` on the the modules that have gone through the FatLTO pipeline. > Is that insufficient? > > Also, as a best practice, do you know if there are specific types of changes > to `clang` that imply that we'd need to test `-S`? Sorry for all the > questions, but I'm trying to get a bit better in how I structure my patches > and approach testing. Ah, sorry for my inaccurate suggestion. You are right. To test `clang/lib/Driver/Driver.cpp`, we need to test the cc1 options of these driver options: * `-S -ffat-lto-objects -flto` * `-S -emit-llvm -ffat-lto-objects -flto` * `-c -ffat-lto-objects -flto` (already tested) test/CodeGen doesn't need more tests. ================ Comment at: clang/test/Driver/fat-lto-objects.c:10 +// CHECK-CC-NOLTO-NOT: -ffat-lto-objects +// CHECK-CC-NOLTO-NOT: warning: argument unused during compilation: '-ffat-lto-objects' + ---------------- paulkirth wrote: > MaskRay wrote: > > This NOT pattern has no effect as warnings are usually emitted before -cc1. > > > > You can use `--implicit-check-not=warning:` > I'm happy to try that suggestion, but isn't this testing the generated `cc1` > command from the driver(e.g., because we're using `%clang` and not > `%clang_cc1`)? I //think// that should still produce the warning, shouldn't > it? > > It's been a while since I made the patch, but I recall writing this test, > having it fail, and then applying the change to stop it from being an `unused > argument`... > > Also, thanks for all the great feedback on testing. Despite writing > many(well, more than a few at least) `lit` tests, I'm still surprised by lots > of behaviors, and your suggestions have been very helpful. Yes, test/Driver just tests how Clang Driver generates warnings/errors and cc1 options. If there is a warning, it will be before the cc1 line, so `// CHECK-CC-NOLTO-NOT: warning: argument unused during compilation: '-ffat-lto-objects'` after cc1 will have no effect. ``` % myclang -flto -ffat-lto-objects -pie -c a.cc '-###' clang version 17.0.0 Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /tmp/Debug/bin clang: warning: argument unused during compilation: '-pie' [-Wunused-command-line-argument] (in-process) "/tmp/Debug/bin/clang-17" "-cc1" ... ``` Actually, we usually don't want to check that a certain warning doesn't occur. Rather, we want to test no warning occurs, as it's easy to adjust a warning message and get stale tests. In this case, I think I find a better test strategy: -fdriver-only -v -Werror. `-fdriver-only` ensures that an error changes the exit code and lit does check the exit code, fulfilling our testing requirement. ``` % myclang -fdriver-only -v -Werror -flto -ffat-lto-objects -pie -fdriver-only -c a.cc clang: error: argument unused during compilation: '-pie' [-Werror,-Wunused-command-line-argument] % echo $? 1 ``` > Also, thanks for all the great feedback on testing. Despite writing > many(well, more than a few at least) lit tests, I'm still surprised by lots > of behaviors, and your suggestions have been very helpful. NP! Glad to help. I have spent too much time fiddling with driver options... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146777/new/ https://reviews.llvm.org/D146777 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
