[llvm-bugs] [Bug 52110] New: InstCombine nonnull annotation and free hoisting interacts badly

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=52110

            Bug ID: 52110
           Summary: InstCombine nonnull annotation and free hoisting
                    interacts badly
           Product: libraries
           Version: trunk
          Hardware: PC
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: Scalar Optimizations
          Assignee: unassignedbugs at nondot.org
          Reporter: smeenai at fb.com
                CC: johannes at jdoerfert.de, lebedev.ri at gmail.com,
                    llvm-bugs at lists.llvm.org, llvm-dev at redking.me.uk,
                    nikita.ppv at gmail.com, nunoplopes at sapo.pt,
                    quentin.colombet at gmail.com

InstCombine attaches a nonnull attribute to a parameter it can prove is
non-null, and at -Oz, it also hoists a call to free above a null check if it's
profitable for size (https://github.com/llvm/llvm-project/commit/3b2db0bcd397).
The combination of the two causes problems when we mark a call to free as
having a nonnull argument when it's guarded by a null check, and then hoist the
free above the null check which was required for the nonnull annotation to be
correct. For example, for the following C code:

void free(void *);
void f(void *p) { free(p); }
void g(void *p) { if (p) f(p); }

clang -Oz -S -emit-llvm produces:

define dso_local void @f(i8* nocapture %p) local_unnamed_addr #0 {
entry:
  tail call void @free(i8* %p) #2
  ret void
}

declare dso_local void @free(i8* nocapture noundef) local_unnamed_addr #1

define dso_local void @g(i8* %p) local_unnamed_addr #0 {
entry:
  tail call void @free(i8* nonnull %p) #3
  ret void
}

The IR for g retains the nonnull attribute on the argument to free, but the
null check that made this attribute be valid is removed. This can then cause
problems for callers of g with a null argument (which should have been valid);
e.g. SimplifyCFG will remove valid checks after
https://reviews.llvm.org/D94180, since it'll consider calling g with null to be
UB.

(I can't repro this with C if I call free directly from g; I need the
indirection through f to demonstrate the issue. That also matches the structure
of our actual code which ran into this issue.)

I can think of a couple of ways to fix this, and I'd appreciate advice on the
best approach:
1. Don't hoist a free above a null check if its argument is marked nonnull. The
downside is that the argument might have been marked nonnull for a reason other
than the null check, and we'd be unnecessarily pessimizing that case.
2. Remove the nonnull attribute from the argument (if present) after hoisting a
free above a null check. I don't know if the cases this pessimizes are better
or worse than 1.
3. Record that the argument to free was marked nonnull because of the null
check, and use that information to decide whether to hoist the free/remove the
attribute after hoisting. I have no idea if this is tractable at all.
4. Reorder InstCombine so that the free hoisting happens before the nonnull
attribute addition. That seems like the cleanest solution if it's feasible, but
I have no idea if it is.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20211007/bbf4504a/attachment.html>