This is an archive of the discontinued LLVM Phabricator instance.

Warn about memcpy'ing non-trivial C structs
ClosedPublic

Authored by ahatanak on Apr 4 2018, 11:58 PM.

Details

Summary

Issue a warning when non-trivial C structs (structs with 'weak' or 'strong' fields) are copied or initialized by calls to memset, bzero, memcpy, and memmove. This is similar to gcc's -Wclass-memaccess warning. It might be better to add support for -Wclass-memaccess and make it cover both C and C++ structs that are non-trivial.

rdar://problem/36124208

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak created this revision.Apr 4 2018, 11:58 PM
rjmccall added inline comments.Apr 5 2018, 12:23 PM
include/clang/AST/NonTrivialCStructTypeVisitor.h
12 ↗(On Diff #141108)

The header comment here was clearly just copied from another file.

I would name this header "NonTrivialTypeVisitor.h"; I don't think the CStruct adds anything, especially because the visitors actually start from types.

30 ↗(On Diff #141108)

Should you have this pass the array type down? And is it really important to do this in the generic visitor? It seems like something you could do in an IRGen subclass.

include/clang/Basic/DiagnosticSemaKinds.td
619

I think this warning group should be -Wnontrivial-memaccess, and maybe -Wclass-memaccess should just be a subgroup of it.

lib/Sema/SemaChecking.cpp
7343

It would be better to just getAsArrayType, for generality purposes.

ahatanak updated this revision to Diff 141291.Apr 6 2018, 1:13 AM
ahatanak marked 3 inline comments as done.

Address review comments.

include/clang/AST/NonTrivialCStructTypeVisitor.h
30 ↗(On Diff #141108)

The subclasses in CGNonTrivialStruct.cpp need the size and the element type of the array to be passed to visitArray, so I think we have to pass the array type to visitArray. I guess it's possible to move this to the subclasses, but then the visit methods in the subclasses have to check whether the type is an array or not. I think we had a discussion on how arrays should be handled in this review: https://reviews.llvm.org/D41228.

But perhaps you have a better idea in mind?

include/clang/Basic/DiagnosticSemaKinds.td
619

Yes, we can create different DiagGroups when support for -Wclass-memaccess (warning for C++ classes) is added.

rjmccall added inline comments.Apr 6 2018, 10:29 AM
include/clang/AST/NonTrivialCStructTypeVisitor.h
30 ↗(On Diff #141108)

Well, you could "override" the visit method in the subclass, e.g.:

template <class Derived, class RetTy = void>
class CGDestructedTypeVisitor : public DestructedTypeVisitor<Derived, RetTy> {
  using super = DestructedTypeVisitor<Derived, RetTy>;

public:
  using super::asDerived;
  using super::visit;

  template <class... Ts>
  RetTy visit(QualType::DestructionKind DK, QualType FT, Ts &&... Args) {
    if (asDerived().getContext().getAsArrayType(FT))
      return asDerived().visitArray(DK, FT, std::forward<Ts>(Args)...);

    return super::visit(DK, FT, std::forward<Ts>(Args)...);
  }
};

It's a bit more boilerplate, but I really feel like the array logic doesn't belong in the generic visitor.

About the array type: I wasn't trying to suggest that you should pass the element type to visitArray, I was suggesting you could just pass the array type as an ArrayType*, since that's what visitArray actually wants.

ahatanak updated this revision to Diff 142691.Apr 16 2018, 2:03 PM
ahatanak marked an inline comment as done.

Move method visitArray to the derived classes and pass the array type to the method instead of the QualType.

include/clang/AST/NonTrivialCStructTypeVisitor.h
30 ↗(On Diff #141108)

I incorporated both of your suggestions. I renamed the overloaded method 'visit' to 'visitWithKind' so that the 'visit' method that doesn't take the DestructionKind doesn't get hidden by the one that takes DestructionKind and is defined in the derived classes. There are a couple of places in CGNonTrivialStruct.cpp that call the former method.

rjmccall added inline comments.Apr 16 2018, 2:59 PM
lib/Sema/SemaChecking.cpp
7574

Indentation seems messed up in these two clauses.

ahatanak updated this revision to Diff 142705.Apr 16 2018, 3:11 PM
ahatanak marked an inline comment as done.

Fix indentation.

rjmccall accepted this revision.Apr 16 2018, 3:17 PM

Thanks; LGTM.

This revision is now accepted and ready to land.Apr 16 2018, 3:17 PM
This revision was automatically updated to reflect the committed changes.