[clang] Extend target_clones tests to exercise declarations that are not definitions.

[tahonermann]

Summary

This change expands the existing code generation test for the 'target_clones'
attribute to cover uses of multiversion function declarations that lack
definitions. The newly added tests trigger a failure in LLVM IR verification
and the test is therefore marked as an expected failure pending a fix. Adding
'-disable-llvm-verifier' to the RUN lines of the test suffices for the test to
pass with the added annotations. The annotations for the new tests demonstrate
an additional deficiency; that a definition for the resolver function is not
emitted if a definition of the 'target_clones' function is not present.

[tahonermann]

clang/test/CodeGen/attr-target-clones.c

3

The code changes needed for the test to again pass are in D122958.

134–138

These checks illustrate the problem that causes the test to fail; resolver declarations are emitted, but definitions are not.

[erichkeane]

FWIW, I dislike this idea of doing tests in separate commits from the patch itself, it makes the review of the patch more difficult, and makes looking through history more difficult.

[tahonermann]

FWIW, I dislike this idea of doing tests in separate commits from the patch itself, it makes the review of the patch more difficult, and makes looking through history more difficult.

Here is my perspective on this. When adding a new feature, I agree that including tests with the code is useful (the tests usually have no meaningful behavior without the new code in that case). I think the motivation is different for bug fixes though, particularly when there is an existing testing gap. Had these tests been added with the initial target_clones work, then the bug fix commit would reveal the precise change in behavior that is the result of the bug fix. I think that is quite valuable both as a review tool and as part of the code history. By adding the test in a separate commit from the fix, it is possible to observe both the previous undesirable behavior as well as precisely what changed with the fix. This also helps to enable someone that is looking to identify a commit responsible for a bug fix (e.g, someone that is looking to resolve a bug report as works-for-me-in-release-X) to definitively identify a commit as exhibiting the expected fix.

I've practiced this approach at other places I've worked and never found it to make the review process more difficult. If you can elaborate on why you feel it makes review more challenging, perhaps there is another way to address that.

[erichkeane]

In D122954#3427406, @tahonermann wrote:

FWIW, I dislike this idea of doing tests in separate commits from the patch itself, it makes the review of the patch more difficult, and makes looking through history more difficult.

Here is my perspective on this. When adding a new feature, I agree that including tests with the code is useful (the tests usually have no meaningful behavior without the new code in that case). I think the motivation is different for bug fixes though, particularly when there is an existing testing gap. Had these tests been added with the initial target_clones work, then the bug fix commit would reveal the precise change in behavior that is the result of the bug fix. I think that is quite valuable both as a review tool and as part of the code history. By adding the test in a separate commit from the fix, it is possible to observe both the previous undesirable behavior as well as precisely what changed with the fix. This also helps to enable someone that is looking to identify a commit responsible for a bug fix (e.g, someone that is looking to resolve a bug report as works-for-me-in-release-X) to definitively identify a commit as exhibiting the expected fix.

I've practiced this approach at other places I've worked and never found it to make the review process more difficult. If you can elaborate on why you feel it makes review more challenging, perhaps there is another way to address that.

I understand it and just disagree. I've NEVER had a tough time 'seeing the previous behavior' vs 'the current behavior', but I DO have the opposite problem: Figuring out what the associated tests are for a patch.

It loses my ability to make sure that your patch covers all of the cases that are important, and it loses my ability to figure out what the patch is doing from the test itself.

Additionally, and I just noticed, this actually disables a LOT of other tests. So in the 'meantime' between the two patches, we are actually losing test coverage thanks to the 'XFAIL'. I think this is unacceptable enough for me to 'request changes'.

[aaron.ballman]

In D122954#3427422, @erichkeane wrote:

In D122954#3427406, @tahonermann wrote:

FWIW, I dislike this idea of doing tests in separate commits from the patch itself, it makes the review of the patch more difficult, and makes looking through history more difficult.

Here is my perspective on this. When adding a new feature, I agree that including tests with the code is useful (the tests usually have no meaningful behavior without the new code in that case). I think the motivation is different for bug fixes though, particularly when there is an existing testing gap. Had these tests been added with the initial target_clones work, then the bug fix commit would reveal the precise change in behavior that is the result of the bug fix. I think that is quite valuable both as a review tool and as part of the code history. By adding the test in a separate commit from the fix, it is possible to observe both the previous undesirable behavior as well as precisely what changed with the fix. This also helps to enable someone that is looking to identify a commit responsible for a bug fix (e.g, someone that is looking to resolve a bug report as works-for-me-in-release-X) to definitively identify a commit as exhibiting the expected fix.

I've practiced this approach at other places I've worked and never found it to make the review process more difficult. If you can elaborate on why you feel it makes review more challenging, perhaps there is another way to address that.

I understand it and just disagree. I've NEVER had a tough time 'seeing the previous behavior' vs 'the current behavior', but I DO have the opposite problem: Figuring out what the associated tests are for a patch.

It loses my ability to make sure that your patch covers all of the cases that are important, and it loses my ability to figure out what the patch is doing from the test itself.

Strong +1 to this. As a reviewer, I want the patch to be making one logical change so that it remains manageable to review, but it needs to be completely self-contained (functional changes, docs, tests, et al) even if that makes the patch a bit bigger. This is especially important because it makes it far easier to revert problematic commits -- if the functional change has an issue, having to separately revert several other related commits is a burden. It also makes git blame more useful when doing code archeology; you can git blame a test to get to the functional changes more quickly.

[tahonermann]

but I DO have the opposite problem: Figuring out what the associated tests are for a patch

I also have that issue, but I don't see the relevance here. The changes in D122958 that fixes the issues revealed by these tests includes the test updates. So that commit reveals exactly which tests are relevant.

It loses my ability to make sure that your patch covers all of the cases that are important, and it loses my ability to figure out what the patch is doing from the test itself.

I'm not following here. There are two distinct issues; a testing gap that is addressed by this review, and a code issue that is addressed by D122958. Addressing these separately is good separation of concerns. Should there be additional tests of declarations that are not definitions? If so, that would be appropriate to discuss in this review?

this actually disables a LOT of other tests. So in the 'meantime' between the two patches, we are actually losing test coverage thanks to the 'XFAIL'.

I do see XFAIL used for such temporary purposes based on a quick review of git log. See commit 9001168cf8b8c85ec9af9b91756b39d2da0130bf for example. I've used this approach elsewhere for many years.

I want the patch to be making one logical change so that it remains manageable to review, but it needs to be completely self-contained (functional changes, docs, tests, et al) even if that makes the patch a bit bigger.

I strongly agree; that is exactly why these commits are split as they are. Both this review and D122958 are self-contained.

This is especially important because it makes it far easier to revert problematic commits -- if the functional change has an issue, having to separately revert several other related commits is a burden.

I agree that would be a burden, but it isn't the case here.

It also makes git blame more useful when doing code archeology; you can git blame a test to get to the functional changes more quickly.

That is exactly what this separation enables. git blame on the tests will get you both the change for the testing gap as well as the functional change that fixes the behavior.

[aaron.ballman]

In D122954#3427676, @tahonermann wrote:

but I DO have the opposite problem: Figuring out what the associated tests are for a patch

I also have that issue, but I don't see the relevance here. The changes in D122958 that fixes the issues revealed by these tests includes the test updates. So that commit reveals exactly which tests are relevant.

It loses my ability to make sure that your patch covers all of the cases that are important, and it loses my ability to figure out what the patch is doing from the test itself.

I'm not following here. There are two distinct issues; a testing gap that is addressed by this review, and a code issue that is addressed by D122958. Addressing these separately is good separation of concerns. Should there be additional tests of declarations that are not definitions? If so, that would be appropriate to discuss in this review?

this actually disables a LOT of other tests. So in the 'meantime' between the two patches, we are actually losing test coverage thanks to the 'XFAIL'.

I do see XFAIL used for such temporary purposes based on a quick review of git log. See commit 9001168cf8b8c85ec9af9b91756b39d2da0130bf for example. I've used this approach elsewhere for many years.

I want the patch to be making one logical change so that it remains manageable to review, but it needs to be completely self-contained (functional changes, docs, tests, et al) even if that makes the patch a bit bigger.

I strongly agree; that is exactly why these commits are split as they are. Both this review and D122958 are self-contained.

This is especially important because it makes it far easier to revert problematic commits -- if the functional change has an issue, having to separately revert several other related commits is a burden.

I agree that would be a burden, but it isn't the case here.

I disagree. If I need to roll back D122958, that means I also need to roll back other changes in the stack because they rely on the ones in the first patch. Alternatively, if the changes aren't related, then use of a patch stack is perhaps not appropriate (then the reviews should be independent, not interdependent).

It also makes git blame more useful when doing code archeology; you can git blame a test to get to the functional changes more quickly.

That is exactly what this separation enables. git blame on the tests will get you both the change for the testing gap as well as the functional change that fixes the behavior.

Again, I disagree. This separation means when I do a git blame, I'll get a commit somewhere in the patch stack. Then I have to go look at the review to find out there were related commits. (This is not hypothetical -- I've run into this frustration a few times in recent history and each time it's been either a patch stack or a follow-up commit due to built bot breakage.) It's a minor annoyance because I can still find the information I need to find, but it's worth keeping in mind.

This may be a cultural thing, but personally, I find patch stacks to be really difficult to review when they're as split apart as this one has been. It's not that your approach isn't defensible -- you've obviously put thought into this! For me, what makes it hard is that I read the code and I go "where's the test associated with this change" or "I wonder if they thought about this edge case", and with patch stacks like this, that involves an easter egg hunt because your thought process to get to the final state may take different paths than mine. And when I am looking at a review in the "middle" of the patch stack, like this one, it gets even harder. In order to understand the context for these test changes, I have to go find four other things first and keep *all* of that context in mind. Eventually, what I wind up doing is opening up a browser tab for each patch in the stack and tab between them when I need context. It's not ideal, but it gets the job done.

In this particular case, the start of the patch stack has functional changes, followed by three NFC changes, followed by this change to test cases. If the previous three patches truly are NFC, then there's no reason why this change to just the tests shouldn't be rolled into the original functional change *or* be rolled into the next patch that has functional changes intended to fix this test. (From looking at the patch stack, I'd have expected 2-3 reviews, not 7)

This isn't to admonish or discourage you from using patch stacks to help split logical things up, but it is to caution against splitting things too much because it does add a cognitive burden to me as a reviewer. Also, other reviewers and contributors may have different feelings on the matter. We don't have strict rules in this area because we want some flexibility with people's workflows (both contributors and reviewers).

[erichkeane]

In D122954#3427676, @tahonermann wrote:

but I DO have the opposite problem: Figuring out what the associated tests are for a patch

I also have that issue, but I don't see the relevance here. The changes in D122958 that fixes the issues revealed by these tests includes the test updates. So that commit reveals exactly which tests are relevant.

The only relationship between the two from the web interface is that you tell me that, or I go through other links. If I have to swap tabs between tests and code, I lose context, or they get separated.

It loses my ability to make sure that your patch covers all of the cases that are important, and it loses my ability to figure out what the patch is doing from the test itself.

I'm not following here. There are two distinct issues; a testing gap that is addressed by this review, and a code issue that is addressed by D122958. Addressing these separately is good separation of concerns. Should there be additional tests of declarations that are not definitions? If so, that would be appropriate to discuss in this review?

DURING review the addition of tests that the thing changes/fixes being in the same review make the mental load less.

this actually disables a LOT of other tests. So in the 'meantime' between the two patches, we are actually losing test coverage thanks to the 'XFAIL'.

I do see XFAIL used for such temporary purposes based on a quick review of git log. See commit 9001168cf8b8c85ec9af9b91756b39d2da0130bf for example. I've used this approach elsewhere for many years.

This XFAIL mechanism is very rarely used in the CFE for this purpose. It is usually more to disable flakey tests or tests that were broken during another patch (or even broken thanks to the but not yet fixed. No matter where you've used this approach before, your strategy here is both not common in the Clang community, AND you have your two reviewers asking you politely to stop. Frankly, 'review presentation' is for the comfort of the reviewer, not the submitter.

I want the patch to be making one logical change so that it remains manageable to review, but it needs to be completely self-contained (functional changes, docs, tests, et al) even if that makes the patch a bit bigger.

I strongly agree; that is exactly why these commits are split as they are. Both this review and D122958 are self-contained.

This one is actually NOT all that self contained (it disables a ton of otherwise functioning tests), and D122958 doesn't include addition lines of the tests that it affects. It actually enables a bunch of tests that worked/behaved correctly even before it. The association between them is vague/unclear.

This is especially important because it makes it far easier to revert problematic commits -- if the functional change has an issue, having to separately revert several other related commits is a burden.

I agree that would be a burden, but it isn't the case here.

It also makes git blame more useful when doing code archeology; you can git blame a test to get to the functional changes more quickly.

That is exactly what this separation enables. git blame on the tests will get you both the change for the testing gap as well as the functional change that fixes the behavior.

As someone who has done a TON of code archeology, your strategy here makes it significantly harder in my experience. Which, in part, is why I'm so against it. From my perspective and experience, this strategy makes Reviews HARDER and archeology HARDER with no decernable benefit to me.

[tahonermann]

Abandoning this review; the changes made here have been squashed into D122958.

With regard to prior comments, it seems we have divergent experience regarding the utility of separating commits as I initially did (I also have a ton of code archeology experience). However, I acknowledge that I am the newcomer to this community and I am content to follow existing community norms (and will lobby for change through appropriate channels (e.g., not within a code review!) if I believe such change would be beneficial).

clang/test/CodeGen/attr-target-clones.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck %s --check-prefixes=LINUX,CHECK
 // RUN: %clang_cc1 -triple x86_64-windows-pc -emit-llvm %s -o - | FileCheck %s --check-prefixes=WINDOWS,CHECK
+// XFAIL: *
 
 // LINUX: $foo.resolver = comdat any
 // LINUX: $foo_dupes.resolver = comdat any
 // LINUX: $unused.resolver = comdat any
 // LINUX: $foo_inline.resolver = comdat any
 // LINUX: $foo_inline2.resolver = comdat any
 
 // WINDOWS: $foo = comdat any
 // WINDOWS: $foo_dupes = comdat any
 // WINDOWS: $unused = comdat any
 // WINDOWS: $foo_inline = comdat any
 // WINDOWS: $foo_inline2 = comdat any
 
 // LINUX: @foo.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @foo.resolver
 // LINUX: @foo_dupes.ifunc = weak_odr ifunc void (), void ()* ()* @foo_dupes.resolver
 // LINUX: @unused.ifunc = weak_odr ifunc void (), void ()* ()* @unused.resolver
 // LINUX: @foo_inline.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @foo_inline.resolver
 // LINUX: @foo_inline2.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @foo_inline2.resolver
+// LINUX: @foo_used_no_defn.ifunc = weak_odr ifunc i32 (), i32 ()* ()* @foo_used_no_defn.resolver
 
 int __attribute__((target_clones("sse4.2, default"))) foo(void) { return 0; }
 // LINUX: define {{.*}}i32 @foo.sse4.2.0()
 // LINUX: define {{.*}}i32 @foo.default.1()
 // LINUX: define i32 ()* @foo.resolver() comdat
 // LINUX: ret i32 ()* @foo.sse4.2.0
 // LINUX: ret i32 ()* @foo.default.1
 

 // LINUX: ret i32 ()* @foo_inline2.default.2
 
 // WINDOWS: define linkonce_odr dso_local i32 @foo_inline2.arch_sandybridge.0() #[[SB:[0-9]+]]
 // WINDOWS: define dso_local i32 @foo_inline2() comdat
 // WINDOWS: musttail call i32 @foo_inline2.arch_sandybridge.0
 // WINDOWS: musttail call i32 @foo_inline2.sse4.2.1
 // WINDOWS: musttail call i32 @foo_inline2.default.2
 
+
+int __attribute__((target_clones("default", "sse4.2")))
+foo_unused_no_defn(void);
+
+int __attribute__((target_clones("default", "sse4.2")))
+foo_used_no_defn(void);
+
+int test_foo_used_no_defn(void) {
+  // LINUX: define {{.*}}i32 @test_foo_used_no_defn()
+  // WINDOWS: define dso_local i32 @test_foo_used_no_defn()
+  return foo_used_no_defn();
+  // LINUX: call i32 @foo_used_no_defn.ifunc()
+  // WINDOWS: call i32 @foo_used_no_defn()
+}
+// LINUX: declare i32 @foo_used_no_defn.default.1()
+// LINUX: declare i32 ()* @foo_used_no_defn.resolver()
+
+// WINDOWS: declare dso_local i32 @foo_used_no_defn.default.1()
+// WINDOWS: declare dso_local i32 @foo_used_no_defn()
+
+
+// Deferred emission of inline definitions.
 // LINUX: define linkonce i32 @foo_inline.arch_sandybridge.0() #[[SB]]
 // LINUX: define linkonce i32 @foo_inline.default.2() #[[DEF]]
 // LINUX: define linkonce i32 @foo_inline.sse4.2.1() #[[SSE42:[0-9]+]]
 
 // WINDOWS: define linkonce_odr dso_local i32 @foo_inline.arch_sandybridge.0() #[[SB]]
 // WINDOWS: define linkonce_odr dso_local i32 @foo_inline.default.2() #[[DEF]]
 // WINDOWS: define linkonce_odr dso_local i32 @foo_inline.sse4.2.1() #[[SSE42:[0-9]+]]
 
 
 // LINUX: define linkonce i32 @foo_inline2.default.2() #[[DEF]]
 // LINUX: define linkonce i32 @foo_inline2.sse4.2.1() #[[SSE42]]
 
 // WINDOWS: define linkonce_odr dso_local i32 @foo_inline2.default.2() #[[DEF]]
 // WINDOWS: define linkonce_odr dso_local i32 @foo_inline2.sse4.2.1() #[[SSE42]]
 
+
 // CHECK: attributes #[[SSE42]] =
 // CHECK-SAME: "target-features"="+crc32,+cx8,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87"
 // CHECK: attributes #[[DEF]] =
 // Don't bother checking features, we verified it is the same as a normal function.
 // CHECK: attributes #[[SB]] =
 // CHECK-SAME: "target-features"="+avx,+crc32,+cx16,+cx8,+fxsr,+mmx,+pclmul,+popcnt,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt"