r224835 - Initialize CodeGeneratorImpl::Ctx in constructor.

[Kostya Serebryany]

On Mon, Dec 29, 2014 at 10:33 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Fri, Dec 26, 2014 at 11:58 AM, Yaron Keren <yaron.keren at gmail.com>
> 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 <dblaikie at gmail.com>:
>>
>>>
>>>
>>> On Thu, Dec 25, 2014 at 9:49 AM, Yaron Keren <yaron.keren at gmail.com>
>>> 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 <dblaikie at gmail.com>:
>>>>
>>>>> 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" <yaron.keren at gmail.com> 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
>>>>>> cfe-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>
>>>>>
>>>>
>>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141229/898fa17c/attachment.html>