kstoimenov added inline comments.
================ Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:33 + +class SanitizerMetadataFactory { + SanitizerMetadataFactory(const SanitizerMetadataFactory &) = delete; ---------------- Not sure if this class follows the 'factory' design pattern, which typically it would have some sort of 'produce' or 'create' method. Maybe SanitizerMatadataWriter or SanitizerMatadataCreator? ================ Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:46-48 + void setASanSpecificMetadata(llvm::GlobalVariable::SanitizerMetadata &Meta, + llvm::GlobalVariable *GV, SourceLocation Loc, + QualType Ty, bool IsDynInit = false); ---------------- This could be private I think. ================ Comment at: compiler-rt/lib/asan/asan_globals.cpp:90 + + DataInfo info; + Symbolizer::GetOrInit()->SymbolizeData(g.beg, &info); ---------------- Outside of the scope of this change. It looks like DataInfo's constructor is calling memset on it's fields. I would have probably designed this differently because now it looks like potential uninitialized when we check for 'info.line != 0'. ================ Comment at: llvm/include/llvm/AsmParser/LLToken.h:205 kw_nest, + kw_no_sanitize, + kw_no_sanitize_address, ---------------- Why do we need to add these to lex? I am surprised that didn't have the need so far. ================ Comment at: llvm/include/llvm/IR/GlobalValue.h:312 + void RemoveSanitizer(GlobalSanitizer S) { Sanitizer &= ~S; } + bool HasSanitizer(GlobalSanitizer S) const { + if (Sanitizer == GlobalSanitizer::NoSanitize) ---------------- I suspect that there might be another place where that logic exists. I am worried that we are duplicating it. Do you might looking for it in the code base? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124493/new/ https://reviews.llvm.org/D124493 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits