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

[David Blaikie]

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:

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