Differential D17776
Improve CHECK-NOT robustness of dllexport tests Authored by wristow on Mar 1 2016, 11:11 AM.
Details
This changes some dllexport tests, to verify that some symbols that We plan to enable dllimport/dllexport support for the PS4, and these
Diff Detail Event TimelineComment Actions ping This isn't a particularly critical change, but I'm planning on submitting a Comment Actions 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@0
...
; CHECK-CL-SAME: /EXPORT:_alias3
; CHECK-CL-SAME: /EXPORT:_weak_alias"
; CHECK-CL-NOT: not_exportedThat 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@0
...
; CHECK-CL-SAME: /EXPORT:_alias3
; CHECK-CL-SAME: /EXPORT:_weak_alias"
; CHECK-CL-NOT: not_exported
; CHECK-CL-NOT: not_definedAnd 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'. Comment Actions ping The issues in these tests are blocking adding support of dllimport/dllexport for PS4. Comment Actions It looks like you have at least 3 independent patches here:
It would be easier to review this if you split it up.
Comment Actions Thanks Sean. The -NOT/-SAME issue was my main goal, but I threw in new coverage for some edge cases and that's completely independent, as you said. I'll separate that out. The camelCase changes are partially intertwined with the -NOT/-SAME issue, in that the test as it stands defines (for example) a function named notExported() and then the -NOT/-SAME checks mistakenly check for the name not_exported. So unless the camelCase issue is fixed, a fix for the -NOT/-SAME portion isn't effective. In any case, fixing the camelCase independently (prior to the -NOT/-SAME fix) will make things clearer, I agree. I've just separated the camelCase aspect out at http://reviews.llvm.org/D18589. I'll update this review after that one is wrapped up. Comment Actions With the camelCase issues now addressed separately, I've updated the test-changes here to only deal with the issues of safely checking that symbols that should not be exported are not in the export table. In doing this, I've changed the Title and the Summary, to reflect the more focused nature of this proposed change. I'll add the additional test-cases that were in my original proposed changes of this Differentlial separately, once the issues here are resolved. | |||||||||||||||||||||||||
This looks like it is actually trying to add new test coverage. Please split that into a separate patch.