r224835 - Initialize CodeGeneratorImpl::Ctx in constructor.

[David Blaikie]

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?

- 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
>>>>>
>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141229/fec1c91f/attachment.html>