On Mon, Dec 29, 2014 at 10:33 AM, David Blaikie <[email protected]> wrote:
> > > On Fri, Dec 26, 2014 at 11:58 AM, Yaron Keren <[email protected]> > wrote: >> >> Let's disregard dumb gcc warnings. >> > > Well that's why I was asking what your motivation for the change was - > usually this sort of cleanup is motivated by some tool diagnosing the code > as 'bad' (when it might not be/isn't). I'd like to address the root cause > there if possible so we set expectations appropriately about what tool > suggestions we are willing to accept code changes for, etc. > > >> Initializing pointers only (not value variables) to nullptr will prevent >> msan detect uninitialized pointer read, but will usually/always? core dump >> instead. Considering msan works on Linux x64 only, and even then msan isn't >> used most of the time, how about creating an LLVM_NULLPTR macro which will >> expand to nothing on msan builds and to nullptr on non-msan builds, used >> like >> > > Chandler - you've usually been a pretty big advocate of not initializing > things that don't need it (to allow static and dynamic tools to properly > detect real issues). Any idea if this is the sort of thing you'd like to > see? (modulo name bikeshedding, etc) > > Alexey - any thoughts on how best to write code defensively while also > giving sanitizers maximum opportunity to find bugs? > imho initializing pointers to nullptr is a good thing. One should not rely on tools to find a bug if the bug can be cheaply found w/o tools (dereference of nullptr is easy to spot) --kcc > > > - David > > >> >> DIBuilder::DIBuilder(Module &m, bool AllowUnresolvedNodes) >> : M(m), VMContext(M.getContext()), TempEnumTypes(LLVM_NULLPTR), >> TempRetainTypes(LLVM_NULLPTR), TempSubprograms(LLVM_NULLPTR), >> TempGVs(LLVM_NULLPTR), >> DeclareFn(LLVM_NULLPTR), ValueFn(LLVM_NULLPTR), >> AllowUnresolvedNodes(AllowUnresolvedNodes) {} >> >> >> >> 2014-12-26 20:04 GMT+02:00 David Blaikie <[email protected]>: >> >>> >>> >>> On Thu, Dec 25, 2014 at 9:49 AM, Yaron Keren <[email protected]> >>> wrote: >>> >>>> msan uninitialized use diagnostic may be more useful to value >>>> variables, as nullptr pointer access will core dump without msan. >>>> Initializing pointers to nullptr is done everywhere in llvm and clang >>>> (search for '(nullptr)') >>>> >>> >>> There are certainly places where initializing to nullptr is necessary >>> (because the null value is used/tested before any write occurs). And I >>> don't doubt there are many places where it's unnecessary & done out of >>> habit/consistency/forward-aovidance of problems (either due to the code >>> being written before we had MSan, etc - or because the author wasn't aware >>> of the tradeoffs). >>> >>> >>>> such as >>>> >>>> DIBuilder::DIBuilder(Module &m, bool AllowUnresolvedNodes) >>>> : M(m), VMContext(M.getContext()), TempEnumTypes(nullptr), >>>> TempRetainTypes(nullptr), TempSubprograms(nullptr), >>>> TempGVs(nullptr), >>>> DeclareFn(nullptr), ValueFn(nullptr), >>>> AllowUnresolvedNodes(AllowUnresolvedNodes) {} >>>> >>>> isn't init to nullptr the "right" thing to do? >>>> >>> >>> Not necessarily. This isn't a hard-and-fast rule, but we've certainly >>> pushed back on initializing variables that don't need initialization when >>> it was motivated by silencing a tool that wasn't smart enough to know that >>> these initializations weren't needed (such as GCC's warnings). >>> >>> >>>> >>>> >>>> 2014-12-25 18:46 GMT+02:00 David Blaikie <[email protected]>: >>>> >>>>> Any particular reason? It's sometimes handy to leave things >>>>> uninitialized if they stents intended to be read until after some other >>>>> write - in that way, msan can catch bugs when that constraint is violated. >>>>> On Dec 25, 2014 3:41 AM, "Yaron Keren" <[email protected]> wrote: >>>>> >>>>>> Author: yrnkrn >>>>>> Date: Thu Dec 25 05:38:15 2014 >>>>>> New Revision: 224835 >>>>>> >>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=224835&view=rev >>>>>> Log: >>>>>> Initialize CodeGeneratorImpl::Ctx in constructor. >>>>>> >>>>>> >>>>>> Modified: >>>>>> cfe/trunk/lib/CodeGen/ModuleBuilder.cpp >>>>>> >>>>>> Modified: cfe/trunk/lib/CodeGen/ModuleBuilder.cpp >>>>>> URL: >>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ModuleBuilder.cpp?rev=224835&r1=224834&r2=224835&view=diff >>>>>> >>>>>> ============================================================================== >>>>>> --- cfe/trunk/lib/CodeGen/ModuleBuilder.cpp (original) >>>>>> +++ cfe/trunk/lib/CodeGen/ModuleBuilder.cpp Thu Dec 25 05:38:15 2014 >>>>>> @@ -59,7 +59,7 @@ namespace { >>>>>> CodeGeneratorImpl(DiagnosticsEngine &diags, const std::string& >>>>>> ModuleName, >>>>>> const CodeGenOptions &CGO, llvm::LLVMContext& >>>>>> C, >>>>>> CoverageSourceInfo *CoverageInfo = nullptr) >>>>>> - : Diags(diags), CodeGenOpts(CGO), HandlingTopLevelDecls(0), >>>>>> + : Diags(diags), Ctx(nullptr), CodeGenOpts(CGO), >>>>>> HandlingTopLevelDecls(0), >>>>>> CoverageInfo(CoverageInfo), >>>>>> M(new llvm::Module(ModuleName, C)) {} >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>> >>>> >>> > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
