[PATCH] D146777: [clang] Preliminary fat-lto-object support

[Fangrui Song via Phabricator via cfe-commits]

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