[PATCH] D38658: [cfi-verify] Add an interesting unit test where undef search length changes result.

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:654
+  uint64_t PrevSearchLengthForUndef = SearchLengthForUndef;
+  SearchLengthForUndef = 5;
   EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 9));
----------------
pcc wrote:
> hctim wrote:
> > pcc wrote:
> > > Maybe `SearchLengthForUndef` should be a field on `FileAnalysis`?
> > My thoughts are that `SearchLengthForUndef` should be a singleton across all instances of `FileAnalysis`. It just so happens that `FileAnalysis` is always a singleton (apart from these unit tests), and I'm not sure that duplicating this data is neccessary. WDYT?
> Up to you, but it seems that making it a field would make your tests less error-prone, which to me means that it's probably the "right" architecture.
I think I'd prefer to have the flag closer to the place that it's used, rather than be a flag that's copied into each `FileAnalysis` instance at runtime and then passed through options injection into `GraphBuilder`. I think it makes the usage of it too far from the definition for somehting that (in a non-test environment) is unchanged after startup.

Feel free to overwrite me on this though, I'll follow it up in a further patch if that's what you desire :)


https://reviews.llvm.org/D38658