[cfe-commits] r157483 - in /cfe/trunk: lib/CodeGen/CGCall.cpp test/CodeGen/alloc_size.c

[Evan Cheng]

The primary concerns I have seen so far are "it doesn't serve the needs for all memory safety techniques". Did I miss some other specific concern about alloc_size attribute?

I think I should address the recent discussions the LLVM side wrt to the bounds checking pass. I've been following the discussion but thought Nuno have done a good job defending his decisions. But I guess I should clarify the context. Memory safety is a big field, ASAN, SafeCode, and -fbounds-checking are related, but they do not serve the same audience. We're (Nuno is interning for me) looking to implement something light weight that can be used even for kernel code. Given the differences in goals, it seems highly unlikely the various parties will reach a consensus design anytime soon. We thought the current implementation provides an incremental new functionality that has already proven useful (i.e. it has already found issues in php and icu), and it doesn't prevent future work in this area. If a better proposal comes along later, they we'll adopt it. For now, Nuno has to take the lead on implementing the feature.

The primary focus is on the backend analysis so we'd like to make minimum changes to Clang. alloc_size appears to serves as a good way to annotate user-implemented allocation functions. And gcc compatibility may not be a strong advantage, but it at least provide some familiarity. It's not meant to be a big feature. Does it really need to be developed locally first?

Evan

On May 25, 2012, at 3:27 PM, Douglas Gregor <dgregor at apple.com> wrote:

> 
> On May 25, 2012, at 3:23 PM, Nuno Lopes <nunoplopes at sapo.pt> wrote:
> 
>> There has been some discussion on both the bounds checking and the alloc_size proposals on the various mailing lists (dev & commits) for the past month.
>> Yes, the design is not finished. But I've been making changes to the implementation to accommodate the feedback.
> 
> When there are legitimate concerns about the design and implementation of a feature, we prefer that you work on your patches locally and continue to discuss the design on the mailing lists. When the community has reached a general consensus that this is, in fact, the right way to go, the patches can go into the tree.
> 
> 	- Doug
> 
>> Nuno
>> 
>> 
>> Citando Chandler Carruth <chandlerc at google.com>:
>> 
>>> Nuno, this patch didn't get reviewed before commit. The LLVM review is
>>> ongoing, and seems to indicate the solution is not yet ready. Folks have
>>> also raised serious concerns about the implementation strategy here.
>>> 
>>> While supporting the alloc_size attribute at least enough to parse it and
>>> reject invalid formulations seems good for GCC compatibility, adding code
>>> generation and LLVM support to it is adding a significant amount of
>>> maintenance for this feature, and there has been no discussion or general
>>> consensus that this functionality's design is the right long-term solution
>>> for Clang/LLVM. In fact, there was no discussion at all until Kostya
>>> started his email thread, which I don't think you've responded to yet.
>>> 
>>> I would like to suggest you not continue committing (especially without
>>> review) patches toward a system that hasn't been designed and supported by
>>> the larger community. I think you really need to take time to engage the
>>> rest of the open source group in your efforts, and figure out what the
>>> correct design and implementation strategy is here.
>>> 
>>> I've CC'ed Chris and Doug as they may feel differently. =] I would be very
>>> interested in their feedback at least about what is the best process.
>>> -Chandler
>>> 
>>> On Fri, May 25, 2012 at 10:04 AM, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>>> 
>>>> Author: nlopes
>>>> Date: Fri May 25 12:04:42 2012
>>>> New Revision: 157483
>>>> 
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=157483&view=rev
>>>> Log:
>>>> add CodeGen support for the alloc_size attribute
>>>> 
>>>> Added:
>>>>  cfe/trunk/test/CodeGen/alloc_size.c
>>>> Modified:
>>>>  cfe/trunk/lib/CodeGen/CGCall.cpp
>>>> 
>>>> Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=157483&r1=157482&r2=157483&view=diff
>>>> 
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
>>>> +++ cfe/trunk/lib/CodeGen/CGCall.cpp Fri May 25 12:04:42 2012
>>>> @@ -2087,6 +2087,23 @@
>>>> CS.setAttributes(Attrs);
>>>> CS.setCallingConv(static_cast<llvm::CallingConv::ID>(CallingConv));
>>>> 
>>>> +  // add metadata for __attribute__((alloc_size(foo)))
>>>> +  if (TargetDecl) {
>>>> +    if (const AllocSizeAttr* Attr = TargetDecl->getAttr<AllocSizeAttr>())
>>>> {
>>>> +      std::vector<llvm::Value*> Args;
>>>> +      llvm::IntegerType *Ty =
>>>> llvm::IntegerType::getInt32Ty(getLLVMContext());
>>>> +      bool isMethod = isa<CXXMethodDecl>(TargetDecl);
>>>> +
>>>> +      for (AllocSizeAttr::args_iterator I = Attr->args_begin(),
>>>> +           E = Attr->args_end(); I != E; ++I) {
>>>> +        Args.push_back(llvm::ConstantInt::get(Ty, *I + isMethod));
>>>> +      }
>>>> +
>>>> +      llvm::MDNode *MD = llvm::MDNode::get(getLLVMContext(), Args);
>>>> +      CS.getInstruction()->setMetadata("alloc_size", MD);
>>>> +    }
>>>> +  }
>>>> +
>>>> // In ObjC ARC mode with no ObjC ARC exception safety, tell the ARC
>>>> // optimizer it can aggressively ignore unwind edges.
>>>> if (CGM.getLangOpts().ObjCAutoRefCount)
>>>> 
>>>> Added: cfe/trunk/test/CodeGen/alloc_size.c
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/alloc_size.c?rev=157483&view=auto
>>>> 
>>>> ==============================================================================
>>>> --- cfe/trunk/test/CodeGen/alloc_size.c (added)
>>>> +++ cfe/trunk/test/CodeGen/alloc_size.c Fri May 25 12:04:42 2012
>>>> @@ -0,0 +1,11 @@
>>>> +// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s
>>>> +
>>>> +void *my_recalloc(void *, unsigned, unsigned)
>>>> __attribute__((alloc_size(2,3)));
>>>> +
>>>> +// CHECK: @f
>>>> +void* f() {
>>>> +  // CHECK: call i8* @my_recalloc{{.*}}, !alloc_size !0
>>>> +  return my_recalloc(0, 11, 27);
>>>> +}
>>>> +
>>>> +// CHECK: !0 = metadata !{i32 1, i32 2}
>>>> 
>