[PATCH] D17776: Improve CHECK-NOT robustness of dllimport/dllexport tests

[Warren Ristow via llvm-commits]

wristow added a comment.

To clarify the motivation for the CHECK-NOT-style changes (that is, adding some new RUN lines with --check-prefix=NOTEXPORTED, rather than using CHECK-CL-NOT to verify some functions aren't exported), looking at "llvm/test/CodeGen/X86/dllexport.ll", the original version contains a declaration of a function named 'not_defined', with the 'dllexport' attribute.  Since there isn't a definition of 'not_defined', even though it has the 'dllexport' attribute, it isn't exported, and that's the correct/desired behavior.  But there is **no actual check** that 'not_defined' isn't exported.  In fact, the only reference to the name 'not_defined' in the test is the declaration line itself:

  declare dllexport void @not_defined()

It seems like the 'not_defined' aspect of the test was really intended to verify that 'not_defined' doesn't appear in the export table.  Related to this, the test **does **check that 'not_exported' doesn't appear in the export table, via the following approach:

  ; CHECK: .section .drectve
  ; CHECK-CL-NOT: not_exported
  ; CHECK-CL: /EXPORT:_f1
  ; CHECK-CL-SAME: /EXPORT:_f2
  ; CHECK-CL-SAME: /EXPORT:_stdfun at 0
      ...
  ; CHECK-CL-SAME: /EXPORT:_alias3
  ; CHECK-CL-SAME: /EXPORT:_weak_alias"
  ; CHECK-CL-NOT: not_exported

That is, it has a CHECK-CL-NOT for 'not_exported' at the start and the end of the table, with all the expected-to-be-exported items in the middle.  Superficially, it looks like the way to enhance this to also verify that 'not_defined' wasn't incorrectly exported, would be to add two more CHECK-CL-NOT lines, resulting in:

  ; CHECK: .section .drectve
  ; CHECK-CL-NOT: not_exported
  ; CHECK-CL-NOT: not_defined
  ; CHECK-CL: /EXPORT:_f1
  ; CHECK-CL-SAME: /EXPORT:_f2
  ; CHECK-CL-SAME: /EXPORT:_stdfun at 0
      ...
  ; CHECK-CL-SAME: /EXPORT:_alias3
  ; CHECK-CL-SAME: /EXPORT:_weak_alias"
  ; CHECK-CL-NOT: not_exported
  ; CHECK-CL-NOT: not_defined

And it's true that adding those two lines **will pass**.  But it isn't actually verifying that 'not_defined' isn't in the export table.

In particular, if we take the updated checks as shown above (with the two new CHECK-CL-NOT lines for 'not_defined'), and then change the IR for 'not_defined' so that in fact it **is **defined, the test **still **passes.  That is, if the following line:

  declare dllexport void @not_defined()

is replaced with:

  define dllexport void @not_defined() {
  	ret void
  }

then 'not_defined' is exported (as it should be), but the test still passes, even though there are two CHECK-CL-NOT directives for 'not_defined'.  Adding the new RUN lines (with --check-prefix=NOTEXPORTED) was the cleanest way I came up with to deal with this problem.

As an aside, all of this is obscured a bit by a typo in naming.  Specifically, the original test defines an unexported function 'notExported()' and has a few references to it.  But the check that it isn't exported uses the name 'not_exported' instead of 'notExported'.  I left the actual function-name as 'notExported' and changed the relevant checks.  Then for consistency, I changed the function-name 'not_defined' to 'notDefined'.


http://reviews.llvm.org/D17776