================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1346-1347
@@ +1345,4 @@
+// These padding are added to help AddressSanitizer detect
intra-object-overflow
+// bugs. Later in CogeGen an extra code will be emitted to poison these
+// extra paddings.
+bool RecordLayoutBuilder::MayInsertExtraPaddingAfterField(const RecordDecl *RD,
----------------
rsmith wrote:
> Typo. "Later in CodeGen, extra code will [...]"
done
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1348
@@ +1347,3 @@
+// extra paddings.
+bool RecordLayoutBuilder::MayInsertExtraPaddingAfterField(const RecordDecl *RD,
+ const FieldDecl *FD)
{
----------------
rsmith wrote:
> This does not depend on the value of `FD`; perhaps remove `FD` and just call
> it once per class?
Agree. Moved to RecordDecl::MayInsertExtraPadding
In future I may need a separate function to check a particular field (e.g. if
we want paddings only after arrays).
But not in this patch
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1362-1367
@@ +1361,8 @@
+ ReasonToReject = "union";
+ else if (CXXRD->isTriviallyCopyable())
+ ReasonToReject = "trivially copyable";
+ else if (CXXRD->hasSimpleDestructor())
+ ReasonToReject = "has simple DTOR";
+ else if (CXXRD->isStandardLayout())
+ ReasonToReject = "standard layout";
+
----------------
rsmith wrote:
> What's the reason behind picking these rules? It seems like we could be more
> aggressive here and still follow the C++ standard. Is this for C
> compatibility, or people who roll their own memcpy, or something else? I
> guess the ASan-instrumented memcpy can be taught that it's OK to copy
> intra-object redzone?
It's quite hard to teach asan memcpy to avoid intra object redzones w/o extra
false negatives and performance penalty.
But I do want to relax the requirement of non-standard layout, just not in this
CL
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1364-1365
@@ +1363,4 @@
+ ReasonToReject = "trivially copyable";
+ else if (CXXRD->hasSimpleDestructor())
+ ReasonToReject = "has simple DTOR";
+ else if (CXXRD->isStandardLayout())
----------------
rsmith wrote:
> This 'simple destructor' check doesn't seem right. What properties do you
> intend to check for here?
I wanted to limit to classes that have a user defined DTOR, at least in one of
the members, subclasses, etc.
As I say in the comment above, we'll try to relax some of these requirements.
This and other limitations are something that I was able to debug so far, I'll
be removing/relaxing these requirements one by one later.
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1369-1379
@@ +1368,13 @@
+
+ // FIXME (before commit): where should this SpecialCaseList reside?
+ // In RecordLayoutBuilder? In global scope?
+ // Also, can we reuse the existing -fsanitize-blacklist flag to pass
+ // the file path (i.e. the same blacklist file will be used in AST and
+ // CogeGen), or we should have another flag?
+ auto SCL = llvm::SpecialCaseList::createOrDie(
+ "/tmp/blacklist.txt");
+ StringRef Filename =
+ Context.getSourceManager().getFilename(RD->getLocation());
+ if (SCL->inSection("src", Filename))
+ ReasonToReject = "blacklisted by src";
+
----------------
rsmith wrote:
> Please fix =)
>
> I think reusing the same `-fsanitize-blacklist` flag makes sense.
Alexey will commit a separate patch to move fsanitize=blacklist to langopts.
Clearly, I will wait for that and update this CL
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1381-1387
@@ +1380,9 @@
+
+ // FIXME (before commit): what flag to use for debug output?
+ if (0) {
+ const char *ToPrint = ReasonToReject ? ReasonToReject : "ACCEPTED";
+ llvm::errs() << "ASAN_FIELD_PADDING: " << Filename << " " << *RD
+ << "::" << *FD << " : " << ToPrint << "\n";
+ }
+ return ReasonToReject == 0;
+}
----------------
rsmith wrote:
> If you're going to keep this, it should be put behind a remark flag, and
> should use the normal diagnostics machinery.
done (-Rsanitize-address)
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1394-1395
@@ +1393,4 @@
+ SmallVector<FieldDecl *, 16> Fields(D->field_begin(), D->field_end());
+ // FIXME (before commit): is there a way to know that the field is last
+ // w/o this temporary vector?
+ for (size_t i = 0, e = Fields.size(); i < e; i++)
----------------
rsmith wrote:
> This might be easier if you insert padding before fields rather than after
> them.
I've reverted the loop to its previous state because I actually decided to
instrument the last field as well.
This handles cases where we have an array of objects and an overflow from the
last field of one objects touches the other object.
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1397-1398
@@ -1346,1 +1396,4 @@
+ for (size_t i = 0, e = Fields.size(); i < e; i++)
+ LayoutField(Fields[i],
+ i + 1 != e && MayInsertExtraPaddingAfterField(D, Fields[i]));
}
----------------
rsmith wrote:
> Maybe factor out adding the padding from adding the field? I don't think they
> need to be done together, do they?
>
> Also, you shouldn't add padding at the end of the class if it ends in a
> flexible array member.
>> Maybe factor out adding the padding from adding the field? I don't think
>> they need to be done together, do they?
Err. Not sure here. It looks like it fits here naturally because we change the
FieldSize which then affect the computation if the type's size, etc
>> Also, you shouldn't add padding at the end of the class if it ends in a
>> flexible array member.
Added the check for !FieldSize.isZero()
================
Comment at: lib/CodeGen/CGClass.cpp:695
@@ -694,1 +694,3 @@
+// Emit code in CTOR (Prologue==true) or DTOR(Prologue==false)
+// to poison the extra field paddings inserted under
----------------
rsmith wrote:
> "ctor" and "dtor" in lowercase please.
>
> Stripping the poison in the dtor doesn't match C++ semantics: you aren't
> required to run the dtor to reuse the storage as something else, but
> conversely you can't reuse the storage as something else unless you use
> placement-new to construct a new object.
>
> As a compromise to be nicer to existing code, maybe we should strip poison
> both in the dtor and in a new-expression?
We may be able to relax the requirement of unpoisoning in dtor, but not in this
CL.
I've seen at least few places (on in LLVM itself) where we should not
instrument a class w/o dtor.
striping poisin in new-expression is not a solution as it may lead to false
negatives (e.g. when a new-expression is called on an illegal memory)
================
Comment at: lib/CodeGen/CGClass.cpp:703-704
@@ +702,4 @@
+ : cast<CXXDestructorDecl>(CurGD.getDecl())->getParent();
+ if (ClassDecl->hasAttr<PackedAttr>() || ClassDecl->isStandardLayout() ||
+ ClassDecl->isTriviallyCopyable())
+ return;
----------------
rsmith wrote:
> This doesn't match the set of checks you did in
> `MayInsertExtraPaddingAfterField`; in particular, do you need to consider
> unions here?
Agree. I've moved all this logic into RecordDecl::MayInsertExtraPadding and
call it twice, once from RecordLayoutBuilder::LayoutFields and once from
CodeGenFunction::EmitAsanPrologueOrEpilogue
================
Comment at: lib/CodeGen/CGClass.cpp:708-709
@@ +707,4 @@
+ struct SizeAndOffset {
+ uint64_t size;
+ uint64_t offset;
+ };
----------------
rsmith wrote:
> Field names should be capitalized.
done
================
Comment at: lib/CodeGen/CGClass.cpp:719
@@ +718,3 @@
+ for (unsigned i = 0, e = Info.getFieldCount(); i != e; ++i)
+ SSV[i].offset = Info.getFieldOffset(i) / 8;
+
----------------
rsmith wrote:
> Please don't hard-code `8` here; use `ASTContext::toCharUnitsFromBits`
> instead.
done
================
Comment at: lib/CodeGen/CGClass.cpp:737
@@ +736,3 @@
+ // LLVM AddressSanitizer pass may decide to inline them later.
+ llvm::Type *PtrIntTy = Builder.getIntNTy(PtrSize);
+ llvm::Type *Args[2] = {PtrIntTy, PtrIntTy};
----------------
rsmith wrote:
> This is `IntPtrTy` (a member of `CodeGenFunction`).
done
http://reviews.llvm.org/D5687
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
