[FuzzMutate] Allow only sized pointers for the GEP instruction

[igor-laevsky]

Summary

We can't use arbitrary pointers inside of the GEP instruction. What we really need is a pointer which points to the sized object. There are potentially more constraints if we will ever generate more than a single index but this should work for now.

[bogner]

unittests/FuzzMutate/OperationsTest.cpp

16–19 ↗

(On Diff #125520)

Please keep these sorted. clang-format can do that for you.

57–59 ↗

(On Diff #125520)

I don't know gtest all that well, but is putting all of this in an anonymous namespace the right thing to do here? Why not just make parseAssembly static?

179–180 ↗

(On Diff #125520)

Apparently phabricator only shows the diff here in the email and not the we interface, but what's with the arbitrary whitespace changes on these Function::Create calls? According to clang-format, the old way is correct.

294–298 ↗

(On Diff #125520)

I feel like these would be easier to understand / wouldn't need the comments if we create the instructions programmatically like in the other tests in this file rather than parsing it.

[igor-laevsky]

unittests/FuzzMutate/OperationsTest.cpp

57–59 ↗

(On Diff #125520)

I see anonymous namespace in the official gtest examples and in many llvm test sets. So I guess it's the right thing to do.

179–180 ↗

(On Diff #125520)

Hm, didn't notice those. Will run clang format, thanks.

294–298 ↗

(On Diff #125520)

I tried this way but it seemed harder to read, harder to write and more error prone. After all we want to generate llvm ir code and there is no more natural way than to write it directly.

[bogner]

unittests/FuzzMutate/OperationsTest.cpp

57–59 ↗

(On Diff #125520)

Works for me.

294–298 ↗

(On Diff #125520)

I'm not totally convinced. Harder to write maybe, but at least in concert with figuring out what the checks are doing I feel it's easier to read and less error prone. It's not a big deal though, both ways are clear enough.

llvm/trunk/include/llvm/FuzzMutate/OpDescriptor.h

auto Make = [](ArrayRef<Value *>, ArrayRef<Type *> Ts) {
     // TODO: Should these point at something?
     for (Type *T : Ts)
       Result.push_back(UndefValue::get(PointerType::getUnqual(T)));
     return Result;
   };
   return {Pred, Make};
 }
 
+static inline SourcePred sizedPtrType() {
+  auto Pred = [](ArrayRef<Value *>, const Value *V) {
+    if (const auto *PtrT = dyn_cast<PointerType>(V->getType()))
+      return PtrT->getElementType()->isSized();
+    return false;
+  };
+  auto Make = [](ArrayRef<Value *>, ArrayRef<Type *> Ts) {
+    std::vector<Constant *> Result;
+
+    for (Type *T : Ts)
+      if (T->isSized())
+        Result.push_back(UndefValue::get(PointerType::getUnqual(T)));
+
+    return Result;
+  };
+  return {Pred, Make};
+}
+
 static inline SourcePred anyAggregateType() {
   auto Pred = [](ArrayRef<Value *>, const Value *V) {
     return V->getType()->isAggregateType();
   };
   // TODO: For now we only find aggregates in BaseTypes. It might be better to
   // manufacture them out of the base types in some cases.
   auto Find = None;
   return {Pred, Find};

llvm/trunk/lib/FuzzMutate/Operations.cpp

OpDescriptor llvm::fuzzerop::gepDescriptor(unsigned Weight) {
   auto buildGEP = [](ArrayRef<Value *> Srcs, Instruction *Inst) {
     Type *Ty = cast<PointerType>(Srcs[0]->getType())->getElementType();
     auto Indices = makeArrayRef(Srcs).drop_front(1);
     return GetElementPtrInst::Create(Ty, Srcs[0], Indices, "G", Inst);
   };
   // TODO: Handle aggregates and vectors
   // TODO: Support multiple indices.
   // TODO: Try to avoid meaningless accesses.
-  return {Weight, {anyPtrType(), anyIntType()}, buildGEP};
+  return {Weight, {sizedPtrType(), anyIntType()}, buildGEP};
 }
 
 static uint64_t getAggregateNumElements(Type *T) {
   assert(T->isAggregateType() && "Not a struct or array");
   if (isa<StructType>(T))
     return T->getStructNumElements();
   return T->getArrayNumElements();
 }

llvm/trunk/unittests/FuzzMutate/OperationsTest.cpp

 //===- OperationsTest.cpp - Tests for fuzzer operations -------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/FuzzMutate/Operations.h"
+#include "llvm/AsmParser/Parser.h"
 #include "llvm/FuzzMutate/OpDescriptor.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Verifier.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <iostream>
 
 // Define some pretty printers to help with debugging failures.
 namespace llvm {
 void PrintTo(Type *T, ::std::ostream *OS) {
   raw_os_ostream ROS(*OS);

 using testing::Eq;
 using testing::Ge;
 using testing::Each;
 using testing::Truly;
 using testing::NotNull;
 using testing::PrintToString;
 using testing::SizeIs;
 
+namespace {
+std::unique_ptr<Module> parseAssembly(
+    const char *Assembly, LLVMContext &Context) {
+
+  SMDiagnostic Error;
+  std::unique_ptr<Module> M = parseAssemblyString(Assembly, Error, Context);
+
+  std::string ErrMsg;
+  raw_string_ostream OS(ErrMsg);
+  Error.print("", OS);
+
+  assert(M && !verifyModule(*M, &errs()));
+  return M;
+}
+
 MATCHER_P(TypesMatch, V, "has type " + PrintToString(V->getType())) {
   return arg->getType() == V->getType();
 }
+
 MATCHER_P(HasType, T, "") { return arg->getType() == T; }
 
 TEST(OperationsTest, SourcePreds) {
   using namespace llvm::fuzzerop;
 
   LLVMContext Ctx;
 
   Constant *i1 = ConstantInt::getFalse(Ctx);
TEST(OperationsTest, GEP) {
   EXPECT_TRUE(GEPOp.SourcePreds[1].matches({UndefValue::get(Int8PtrTy)},
                                            ConstantInt::get(Int32Ty, 0)));
 
   GEPOp.BuilderFunc({UndefValue::get(Int8PtrTy), ConstantInt::get(Int32Ty, 0)},
                     RI);
   EXPECT_FALSE(verifyModule(M, &errs()));
 }
 
+
+TEST(OperationsTest, GEPPointerOperand) {
+  // Check that we only pick sized pointers for the GEP instructions
+
+  LLVMContext Ctx;
+  const char *SourceCode =
+      "declare void @f()\n"
+      "define void @test() {\n"
+      "  %v = bitcast void ()* @f to i64 (i8 addrspace(4)*)*\n"
+      "  %a = alloca i64, i32 10\n"
+      "  ret void\n"
+      "}";
+  auto M = parseAssembly(SourceCode, Ctx);
+
+  fuzzerop::OpDescriptor Descr = fuzzerop::gepDescriptor(1);
+
+  // Get first basic block of the test function
+  Function &F = *M->getFunction("test");
+  BasicBlock &BB = *F.begin();
+
+  // Don't match %v
+  ASSERT_FALSE(Descr.SourcePreds[0].matches({}, &*BB.begin()));
+
+  // Match %a
+  ASSERT_TRUE(Descr.SourcePreds[0].matches({}, &*std::next(BB.begin())));
+}
+
 TEST(OperationsTest, ExtractAndInsertValue) {
   LLVMContext Ctx;
 
   Type *Int8PtrTy = Type::getInt8PtrTy(Ctx);
   Type *Int32Ty = Type::getInt32Ty(Ctx);
   Type *Int64Ty = Type::getInt64Ty(Ctx);
 
   Type *StructTy = StructType::create(Ctx, {Int8PtrTy, Int32Ty});
EXPECT_TRUE(IVOp.SourcePreds[2].matches({SVal, UndefValue::get(Int32Ty)},
                                           ConstantInt::get(Int32Ty, 1)));
 
   EXPECT_THAT(IVOp.SourcePreds[1].generate({SVal}, {}),
               Each(AnyOf(HasType(Int32Ty), HasType(Int8PtrTy))));
   EXPECT_THAT(
       IVOp.SourcePreds[2].generate({SVal, ConstantInt::get(Int32Ty, 0)}, {}),
       ElementsAre(ConstantInt::get(Int32Ty, 1)));
 }
+
+}