aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/Randstruct.h:34
+
+extern std::string SEED;
+
----------------
void wrote:
> aaron.ballman wrote:
> > Doing this with a global variable is unfortunate; it could make things
> > harder when we multithread the frontend. Can we solve this without a
> > global? (And if not, does this global need to be a `ManagedStatic`?)
> Maybe this could be moved to `LangOpts`?
That's a really good idea, let's go with that approach.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11543
+// Layout randomization warning.
+def cast_from_randomized_struct : Error<
+ "casting from randomized structure pointer type %0 to %1">;
----------------
================
Comment at: clang/lib/AST/Randstruct.cpp:99
+
+ if (FD->isBitField() && !FD->isZeroLengthBitField(Context)) {
+ // Start a bitfield run if this is the first bitfield we have found.
----------------
void wrote:
> aaron.ballman wrote:
> > Oh cool, we do bit-field runs!
> >
> > How should we handle anonymous bit-fields (if anything special should be
> > done for them at all)? People usually use them for padding between
> > bit-fields, so it's not clear to me how to treat them when randomizing.
> Good question! I'm not sure either. On one hand, I'm rather concerned about
> changing bitfield order in general, but it appears to be something that GCC's
> plugin does, so...
>
Yeah, I was really surprised about bit-fields as well. I'm especially concerned
that bit-fields which were [un]carefully straddling allocation unit boundaries
might be rearranged such that subtle atomic bugs and whatnot will be exposed.
If we find these sort of concerns are valid in real world code, we may want to
add a second command line option (off-by-default, perhaps) for enabling
rearranging bit-fields. But for now, I think it's fine to follow GCC's lead
here.
================
Comment at: clang/lib/AST/Randstruct.cpp:179
+
+ SmallVector<FieldDecl *, 64> RandomizedFields;
+
----------------
void wrote:
> aaron.ballman wrote:
> > This one seems a bit large to me; any reason not to use `16` again?
> The code above looks to apply mostly to bitfield runs. This is for all fields
> in a structure. I assumed (without proof) that this will always be larger
> than the bitfield runs. :-)
I think that's a safe assumption and it's probably not worth worrying about
overly much. It's more that 16 bit-fields seemed like it would be large enough
to cover most bit-fields in a class while still being "small", but 64 fields
seems likely to be way larger than most structures will need and isn't very
small.
That said, I don't have a strong feeling here and I think the numbers are
defensible unless we measure something that says otherwise.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2503
+
+ // FIXME: Any way to get a handle to a RecordDecl struct here?
+ clang::randstruct::checkForBadCasts(S, AC);
----------------
void wrote:
> aaron.ballman wrote:
> > No, this function is only called when popping a function scope, and so the
> > only declaration it has access to is the `FunctionDecl`. Or do I
> > misunderstand the question?
> Sounds good to me. Is this the best place for this check then?
I'm not convinced it is the best place for it -- analysis-based warnings are
typically ones that require a CFG to be analyzed and so they are only run once
we know the function body itself is valid (they also typically require the user
to manually enable the checks -- your check runs always, even if the user
hasn't enabled struct randomization, so it walks a lot of ASTs it doesn't need
to). Your check doesn't require a CFG at all and it seems like it can be done
purely when building the AST. I think a better place for this checking is in
SemaCast.cpp in the `CastOperation` class.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121556/new/
https://reviews.llvm.org/D121556
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
