This is an archive of the discontinued LLVM Phabricator instance.

Differential D117922

InferFunctionAttrs: fix test case on aligned_alloc
ClosedPublic

Authored by durin42 on Jan 21 2022, 12:53 PM.

Details

Reviewers
reames
jyknight
Commits
rG6cda6d2f61a6: [test] check strictest attributes possible for InferFunctionAttrs test
Summary

This appears to have all the same attributes as many other functions
in this file, and I think the use of INACCESSIBLEMEMONLY_NOFREE_NOUNWIND
instead of INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN was an
oversight that meant aligned_alloc's attributes were just going
unchecked. This patch corrects the test defect and now the attributes
inferred on aligned_alloc are actually validated, and the test still
passes.

Diff Detail

Event Timeline

durin42 requested review of this revision.Jan 21 2022, 12:53 PM
durin42 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 12:53 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
durin42 added a child revision: D117923: BuildLibCalls: add allocalign attributes for memalign and aligned_alloc.Jan 21 2022, 12:53 PM
durin42 added reviewers: reames, jyknight.Jan 21 2022, 12:54 PM
Harbormaster completed remote builds in B144916: Diff 402087.Jan 21 2022, 3:41 PM
durin42 updated this revision to Diff 402905.Jan 25 2022, 7:04 AM
durin42 edited the summary of this revision. (Show Details)
reames requested changes to this revision.Jan 25 2022, 7:27 AM

It seems rather suspect to have a test change to fix an assertion. Either the IR is invalid - in which case the verifier should reject - or it's valid, and the pass can not crash.

Patch rejected.

p.s. Please provide detail on the assertion you're fixing.

This revision now requires changes to proceed.Jan 25 2022, 7:27 AM
durin42 added a comment.Jan 25 2022, 7:43 AM
In D117922#3269532, @reames wrote:

It seems rather suspect to have a test change to fix an assertion. Either the IR is invalid - in which case the verifier should reject - or it's valid, and the pass can not crash.

Patch rejected.

p.s. Please provide detail on the assertion you're fixing.

As far as I can tell, this assertion was vacuous and never checked, presumably due to someone having replaced INACCESSIBLEMEMONLY_NOFREE_NOUNWIND with INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN and having missed one. Without this patch, the next change in the series doesn't notice the new allocalign() attribute that gets added to aligned_alloc (so it "spuriously" passes).

IOW: this was a buggy test before, and nobody noticed. I'm like 95% sure of that diagnosis.

durin42 added a comment.Jan 25 2022, 7:48 AM

I'm very open to suggestions on how to reword the commit message here, BTW. I thought it was clear, but seems not. :(

jyknight added a comment.Jan 25 2022, 11:35 AM
In D117922#3269582, @durin42 wrote:

I'm very open to suggestions on how to reword the commit message here, BTW. I thought it was clear, but seems not. :(

The miscommunication here is that you meant "Fix a test-case to actually verify the behavior it was supposed to", whereas philip read it as "fixes an assert(...) failure"...by adjusting a test.

durin42 requested review of this revision.Jan 26 2022, 9:14 AM
durin42 updated this revision to Diff 403304.
durin42 added a child revision: D118262: [NFC] compatibility: add test for allocsize attribute.
durin42 removed a child revision: D117923: BuildLibCalls: add allocalign attributes for memalign and aligned_alloc.Jan 26 2022, 9:15 AM
durin42 edited parent revisions, added: D117356: InstructionCombining: avoid eliding mismatched alloc/free pairs; removed: D117921: Attributes: add a new allocalign attribute.Jan 26 2022, 9:19 AM
Harbormaster completed remote builds in B145776: Diff 403304.Jan 27 2022, 4:01 AM
reames added a comment.Jan 27 2022, 12:54 PM
In D117922#3270243, @jyknight wrote:
In D117922#3269582, @durin42 wrote:

I'm very open to suggestions on how to reword the commit message here, BTW. I thought it was clear, but seems not. :(

The miscommunication here is that you meant "Fix a test-case to actually verify the behavior it was supposed to", whereas philip read it as "fixes an assert(...) failure"...by adjusting a test.

If you say so. I see no way to get the former from the patch description, but that's probably the problem.

durin42 updated this revision to Diff 404918.Feb 1 2022, 7:00 AM
durin42 retitled this revision from InferFunctionAttrs: fix assertion on aligned_alloc to InferFunctionAttrs: fix test case on aligned_alloc.
durin42 edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B146892: Diff 404918.Feb 1 2022, 8:19 AM
jyknight accepted this revision.Feb 2 2022, 9:01 AM

Commit title was fixed, so objection was resolved.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 3 2022, 8:45 AM
Closed by commit rG6cda6d2f61a6: [test] check strictest attributes possible for InferFunctionAttrs test (authored by durin42, committed by reames). · Explain Why
This revision was automatically updated to reflect the committed changes.
reames added a commit: rG6cda6d2f61a6: [test] check strictest attributes possible for InferFunctionAttrs test.
durin42 removed a parent revision: D117356: InstructionCombining: avoid eliding mismatched alloc/free pairs.Feb 4 2022, 9:36 AM

Revision Contents

PathSize
llvm/
test/
Transforms/
InferFunctionAttrs/
2 lines

Diff 405667

llvm/test/Transforms/InferFunctionAttrs/annotate.ll

Show First 20 Lines • Show All 213 Lines • ▼ Show 20 Lines
declare float @acoshf(float) declare float @acoshf(float)
; CHECK: declare x86_fp80 @acoshl(x86_fp80) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]] ; CHECK: declare x86_fp80 @acoshl(x86_fp80) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
declare x86_fp80 @acoshl(x86_fp80) declare x86_fp80 @acoshl(x86_fp80)
; CHECK: declare x86_fp80 @acosl(x86_fp80) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]] ; CHECK: declare x86_fp80 @acosl(x86_fp80) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
declare x86_fp80 @acosl(x86_fp80) declare x86_fp80 @acosl(x86_fp80)
; CHECK: declare noalias noundef i8* @aligned_alloc(i64 noundef, i64 noundef) [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND:#[0-9]+]] ; CHECK: declare noalias noundef i8* @aligned_alloc(i64 noundef, i64 noundef) [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN:#[0-9]+]]
declare i8* @aligned_alloc(i64, i64) declare i8* @aligned_alloc(i64, i64)
; CHECK: declare double @asin(double) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]] ; CHECK: declare double @asin(double) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
declare double @asin(double) declare double @asin(double)
; CHECK: declare float @asinf(float) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]] ; CHECK: declare float @asinf(float) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
declare float @asinf(float) declare float @asinf(float)
▲ Show 20 Lines • Show All 850 Lines • Show Last 20 Lines