fmayer added a comment.
Thanks!
================
Comment at: clang/test/CodeGen/hwasan-stack-safety-analysis-asm.c:4
+
+int main(int argc, char **argv) {
+ char buf[10];
----------------
vitalybuka wrote:
> fmayer wrote:
> > vitalybuka wrote:
> > > this patch mostly change code under llvm/ so tests should be also there,
> > > as IR tests
> > >
> > >
> > I don't have strong feelings, but clang/test/CodeGen/lifetime-sanitizer.c
> > is a very similar test, so I think we should either move all of these to
> > llvm/ or add the new ones here to clang/. What do you think?
> That lifetime tests how clang inserts lifetime markers. So it must be in
> clang/ this is from https://reviews.llvm.org/D20759 which is clang only patch.
> Here the only change for clang is forwarded BuilderWrapper.getTargetTriple().
> I don't mind if you keep your tests here, but we also need something which
> tests llvm without clang as you change llvm tranformation.
> Usually if contributor changes code in llvm/, expectation is that check-llvm
> should discover regression. It's not always possible, but that's the goal and
> easy to do with this patch.
Thanks for the explanation. Moved to llvm/.
================
Comment at: llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h:32
+ Triple TargetTriple = {});
PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM);
static bool isRequired() { return true; }
----------------
vitalybuka wrote:
> fmayer wrote:
> > vitalybuka wrote:
> > > Why not from M.getTargetTriple() ?
> > Mostly for consistency with the legacy pass. Either way is fine for me
> > though, what do you prefer?
> I don't know if will cause any issues, but usually most passes get triple
> from the module.
> I prefer we stay consistent with the rest of the code if possible.
>
I'll leave it as is, for consistency within this file, as we need to do it this
way for the new pass manager.
================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:444
+ const StackSafetyGlobalInfo *SSI = nullptr;
+ if (shouldUseStackSafetyAnalysis(TargetTriple, IsOptNull)) {
+ SSI = &MAM.getResult<StackSafetyGlobalAnalysis>(M);
----------------
vitalybuka wrote:
> we usually don't use {} for single line
> also maybe good candidate for ?: operator
I think this would make a bit of a long tenary expression.
================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:390
+ void getAnalysisUsage(AnalysisUsage &AU) const override {
+ if (shouldUseStackSafetyAnalysis(TargetTriple)) {
+ AU.addRequired<StackSafetyGlobalInfoWrapperPass>();
----------------
vitalybuka wrote:
> fmayer wrote:
> > vitalybuka wrote:
> > > why we need to check TargetTriple for that?
> > Because we only need the stack safety analysis if we instrument the stack,
> > which we do not do on x86_64 (see shouldInstrumentStack).
> I see, I forgot about this limitation.
> LGTM, but unconditional is fine as well, assuming we are going to have stack
> instrumentation at some point?
I am not sure we will ever add it for non-LAM x86_64.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105703/new/
https://reviews.llvm.org/D105703
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
