r205664 - Revert "DebugInfo: Place global constants in their appropriate context."

[David Blaikie]

On Sat, Apr 5, 2014 at 11:13 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Sat, Apr 5, 2014 at 11:05 AM, Yunzhong Gao <gaoyunzhong at gmail.com> wrote:
>> Hi David,
>>
>> Please ignore my previous email.
>>
>> We had a local patch that is similar to r205655, and
>> then we ran into a memory corruption. My colleague
>> investigated and found one of the workarounds is to
>> convert the AllGVs list to use TrackingVH, so I thought
>> to share it with you. The "deferred case" does not
>> refer to any particular test case, it means that the
>> patch in r205655 applies to the version of
>> EmitGlobalVariable (there are three of them) that only
>> gets called when the emission of some definitions is
>> deferred; IIRC, if you pass -femit-all-decls, the bug
>> in PR19298 should just go away. Looks like you have
>> fixed the problem in a better way, and you did it so
>> quickly. Sorry for causing you some confusion.
>>
>> Now that you have committed r205655+r205668, do you
>> still need to keep r205651 at all?
>
> Unfortunately, yes. There's still a bug here that will manifest with
> non-function-local statics:

On second thoughts, you might be right - since the memory location
will be emitted before any constant use, probably... I'll have to try
it and see.

>
> If the value and the address of the variable are used, we'll still see
> two global (or namespace) variables. Only two, in both the metadata
> and the final DWARF (unlike prior to my patch when we would see many
> in the metadata list (all deduplicated) and two in the final DWARF).
> This might only happen if the memory usage comes after the constant
> value. If it's the other way around the deduplicating logic in the
> constant case might catch the mem+const case too...
>
> That'll still need to be addressed (we should favor the address
> version if we're emitting it, otherwise emit the constant value -
> that's what GCC does) somehow. Looking up the existing constant value
> and replacing it with the memory value most likely. But that requires
> looking into the global variable map held within DIBuilder...
>
>>
>> - Gao
>>
>> On Sat, Apr 5, 2014 at 12:51 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Sat, Apr 5, 2014 at 12:23 AM, Yunzhong Gao <gaoyunzhong at gmail.com> wrote:
>>>> Hi David,
>>>> Just wondering,
>>>> Did you try changing AllGVs to use TrackingVH since they can be updated
>>>> again (multiple forward declarations) in the deferred case?
>>>
>>> I'm not sure which case you're referring to - but no, I've not tried
>>> using TrackingVH as yet...
>>>
>>> Do you have a test case? (or a patch thread to ping I should be looking at?)
>>>
>>>> - Gao
>>>>
>>>>
>>>> On Fri, Apr 4, 2014 at 8:39 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>> Author: dblaikie
>>>>> Date: Fri Apr  4 22:39:29 2014
>>>>> New Revision: 205664
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=205664&view=rev
>>>>> Log:
>>>>> Revert "DebugInfo: Place global constants in their appropriate context."
>>>>>
>>>>> This reverts commit r205655.
>>>>>
>>>>> Breaks the compiler-rt build with an assertion failure in LLVM...
>>>>> reverting while I investigate.
>>>>>
>>>>> Modified:
>>>>>     cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>>>>>     cfe/trunk/test/CodeGenCXX/debug-info.cpp
>>>>>
>>>>> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=205664&r1=205663&r2=205664&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
>>>>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Fri Apr  4 22:39:29 2014
>>>>> @@ -3230,11 +3230,8 @@ void CGDebugInfo::EmitGlobalVariable(con
>>>>>    // Do not emit separate definitions for function local const/statics.
>>>>>    if (isa<FunctionDecl>(VD->getDeclContext()))
>>>>>      return;
>>>>> -  llvm::DIDescriptor DContext =
>>>>> -      getContextDescriptor(dyn_cast<Decl>(VD->getDeclContext()));
>>>>>    llvm::DIGlobalVariable GV = DBuilder.createStaticVariable(
>>>>> -      DContext, Name, StringRef(), Unit, getLineNumber(VD->getLocation()), Ty,
>>>>> -      true, Init,
>>>>> +      Unit, Name, Name, Unit, getLineNumber(VD->getLocation()), Ty, true, Init,
>>>>>        getOrCreateStaticDataMemberDeclarationOrNull(cast<VarDecl>(VD)));
>>>>>    DeclCache.insert(std::make_pair(VD->getCanonicalDecl(), llvm::WeakVH(GV)));
>>>>>  }
>>>>>
>>>>> Modified: cfe/trunk/test/CodeGenCXX/debug-info.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info.cpp?rev=205664&r1=205663&r2=205664&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/test/CodeGenCXX/debug-info.cpp (original)
>>>>> +++ cfe/trunk/test/CodeGenCXX/debug-info.cpp Fri Apr  4 22:39:29 2014
>>>>> @@ -83,16 +83,9 @@ foo func(foo f) {
>>>>>  // CHECK: [[FUNC:![0-9]*]] = {{.*}} metadata !"_ZN7pr147634funcENS_3fooE", i32 {{[0-9]*}}, metadata [[FUNC_TYPE:![0-9]*]], {{.*}} ; [ DW_TAG_subprogram ] {{.*}} [def] [func]
>>>>>  }
>>>>>
>>>>> -namespace local_const {
>>>>> -const wchar_t lc_c = L'x';
>>>>> -}
>>>>> -
>>>>> -// CHECK: metadata [[LOCAL_CONST:![0-9]*]], metadata !"lc_c", {{.*}}; [ DW_TAG_variable ] [lc_c]
>>>>> -// CHECK: [[LOCAL_CONST]] = {{.*}}; [ DW_TAG_namespace ] [local_const]
>>>>> -
>>>>>  void foo() {
>>>>>    const wchar_t c = L'x';
>>>>> -  wchar_t d = c + local_const::lc_c;
>>>>> +  wchar_t d = c;
>>>>>  }
>>>>>
>>>>>  // CHECK-NOT: ; [ DW_TAG_variable ] [c]
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits