paulkirth added a comment.
In D146777#4499608 <https://reviews.llvm.org/D146777#4499608>, @MaskRay wrote: > Thanks for the update. Can you add a comment for the `-funified-lto` > combination? It's unclear what it does... ugh, I had put comments above each compiler invocation, but forgot that `#` makes lines ignored in the commit message. Thanks for pointing that out, as I hadn't noticed after updating. > `clang -flto=thin -ffat-lto-objects -funified-lto -fuse-ld=lld foo.c` > > I've left some comments about missing test coverage. ah, sorry, for some reason I read your last comment as `test locally to confirm it works` instead of the obvious `write a proper test`. I will correct that shortly. ================ 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 ---------------- 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. ================ 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' + ---------------- 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. 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
