[PATCH] Add a DWOId field to DICompileUnit (so DWARF skeleton CUs can be expression in IR).

[David Blaikie]

On Wed, May 6, 2015 at 2:50 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On May 6, 2015, at 2:44 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, May 6, 2015 at 2:41 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> On May 6, 2015, at 2:38 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Wed, May 6, 2015 at 2:25 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>
>>>
>>> On May 5, 2015, at 1:23 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>
>>>
>>> On Tue, May 5, 2015 at 11:52 AM, Adrian Prantl <aprantl at apple.com>
>>> wrote:
>>>
>>>>
>>>> On May 5, 2015, at 9:42 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, May 5, 2015 at 8:35 AM, Adrian Prantl <aprantl at apple.com>
>>>> wrote:
>>>>
>>>>> David, I’m not entirely sure if you are ok with the naming of the
>>>>> field as is.. is this good to commit from your side?
>>>>>
>>>>
>>>> It's a bit generic/misleading given that for the common case of plain
>>>> Fission the DWO ID is generated based on the DWARF in the backend, so it
>>>> doesn't make sense to carry it as part of the metadata because it cannot be
>>>> known then.
>>>>
>>>> What's the particular part of the module debug info this will be used
>>>> for? Your comment says it's specifically for the skeleton side (so we'll
>>>> take the module ID in the frontend and emit a skeleton CU into the
>>>> metadata)? What about the concrete side for the module's debug info? (it'll
>>>> need the same dwo ID)
>>>>
>>>>
>>>> I casually mentioned that in one of the emails in the other thread; the
>>>> idea is that when emitting a clang module the backend computes the regular
>>>> DWARF DWOId for it. When emitting an external type reference, the frontend
>>>> is using libDebugInfo to extract the DWOId from the module and emits a
>>>> skeleton CU with pcm filename and DWOId.
>>>>
>>>
>>> That seems a bit heavy... I thought we discussed with Richard Smith that
>>> the module has an actual identifier we could use here?
>>>
>>>
>>> I realize now that I haven't communicated this problem back to the list
>>> after discovering it, and I apologize for that: It is true that each type
>>> has a unique TypeId in the module,
>>>
>>
>> I don't think I was suggesting using a module based type id - I figured
>> we'd use the type IDs that type units already use (a hash of the fully
>> qualified name - same as the type uniquing in debug info metadata that was
>> implemented years ago (well the type uniquing uses the mangled name itself
>> - I just hashed that to create the type identifier for the comdat section
>> and type signature for type units))
>>
>> In any case, I wasn't asking about the type identifier, I was asking
>> about the module identifier (the DWO ID).
>>
>>
>> Oh. The module does have a module id, but that value is the result of
>> running the random number generator in a loop until we hit a non-zero
>> value, so it’s not exactly resilient against rebuilding the module.
>>
>
> OK, that sounds like the module ID that Chandler came across - apparently
> it was added to work around instability in the module output. Chandler's
> since fixed the module instability and removed this under a flag (I guess
> the flag is still on by default for implicit modules builds).
>
> Though I'm confused - wouldn't rebuilding the module /want/ to change the
> identifier so these don't match anymore? The module might have different
> things compared to what the original code was built against...
>
>
> Remember that on Darwin the module cache is this (sort of) global shared
> space that in theory could have been cleared by the time llvm-dsymutil
> wants to link the debug info.
> In that case I want to be able to rebuild the module and have the exact
> same DWO ID or error.
>

Using the DWO ID seems insufficient if you want the ASTs to all match up -
DWARF doesn't have all the information in the AST, it's lossy. Which means
you could have a change to the ASTs that makes the modules incompatible
that wouldn't be reflected in a hash of the DWARF.


> In other situations the requirements are less strict. If I’m just
> debugging without a dSYM I could imagine being happy with a best effort
> after rebuilding a module that is “close enough”.
>
> -- adrian
>
>
>
>>
>> -- adrian
>>
>>
>>
>>
>>> but this is just a integer counter that is not necessarily stable across
>>> rebuilds. With modules becoming more deterministic, an id should survive a
>>> rebuild of the module, but if the header file the module is built with is
>>> altered in any way, the ids could be shuffled. It is more resilient (but
>>> still fast enough) to look up a type by decl context + name.
>>>
>>> -- adrian
>>>
>>> While I'm all for incremental development, a little context of the path
>>>> you've got in mind may be helpful.
>>>>
>>>>
>>>> Right, the examples in cfe-dev were always DWARF and never showed the
>>>> IR:
>>>> A skeleton CU emitted by the frontend would be represented as a
>>>>   !0 = DICompileUnit(file: MDFile(“/path/to”, “module.pcm”), dwoId:
>>>> ABCD1234)
>>>> and is emitted as a compile_unit with an AT_dwo_name and an AT_dwo_id
>>>> by the backend.
>>>>
>>>
>>> OK.
>>>
>>>
>>>>
>>>> -- adrian
>>>>
>>>>
>>>> - David
>>>>
>>>>
>>>>>
>>>>> -- adrian
>>>>>
>>>>> > On May 4, 2015, at 9:04 PM, Duncan P. N. Exon Smith <
>>>>> dexonsmith at apple.com> wrote:
>>>>> >
>>>>> >
>>>>> >> On 2015 May 4, at 20:17, Adrian Prantl <aprantl at apple.com> wrote:
>>>>> >>
>>>>> >> Now with autoupgrade testcase.
>>>>> >>
>>>>> >>
>>>>> >> http://reviews.llvm.org/D9488
>>>>> >>
>>>>> >> Files:
>>>>> >> include/llvm/IR/DIBuilder.h
>>>>> >> include/llvm/IR/DebugInfoMetadata.h
>>>>> >> lib/AsmParser/LLParser.cpp
>>>>> >> lib/Bitcode/Reader/BitcodeReader.cpp
>>>>> >> lib/Bitcode/Writer/BitcodeWriter.cpp
>>>>> >> lib/IR/AsmWriter.cpp
>>>>> >> lib/IR/DIBuilder.cpp
>>>>> >> lib/IR/DebugInfoMetadata.cpp
>>>>> >> lib/IR/LLVMContextImpl.h
>>>>> >> test/Assembler/mdcompileunit.ll
>>>>> >> test/Bitcode/DICompileUnit-upgrade.test
>>>>> >> test/Bitcode/Inputs/DICompileUnit-no-DWOId.bc
>>>>> >> unittests/IR/MetadataTest.cpp
>>>>> >>
>>>>> >> EMAIL PREFERENCES
>>>>> >> http://reviews.llvm.org/settings/panel/emailpreferences/
>>>>> >> <D9488.24927.patch>
>>>>> >
>>>>> > LGTM with some changes to the autoupgrade test.
>>>>> >
>>>>> >> Index: test/Bitcode/DICompileUnit-upgrade.test
>>>>> >> ===================================================================
>>>>> >> --- /dev/null
>>>>> >> +++ test/Bitcode/DICompileUnit-upgrade.test
>>>>> >
>>>>> > Can you be more descriptive?  Perhaps dicompileunit-no-dwoid.ll.
>>>>> > (Feel free to use the camel-case filename; I personally avoid
>>>>> > capitals but clearly there's no harm.)
>>>>> >
>>>>> >> @@ -0,0 +1,6 @@
>>>>> >> +RUN: llvm-dis %p/Inputs/DICompileUnit-no-DWOId.bc -o - | FileCheck
>>>>> %s
>>>>> >
>>>>> > %p was undocumented last I checked.  I recommend the documented %S
>>>>> > when this sort of functionality is necessary.
>>>>> >
>>>>> > But I wouldn't even use the `Inputs` folder here.  The usual bitcode
>>>>> > upgrade tests say:
>>>>> >
>>>>> >    ; RUN: llvm-dis < %s.bc | FileCheck %s
>>>>> >    ; RUN: verify-uselistorder < %s.bc
>>>>> >
>>>>> > and just drop a .ll.bc file next to a .ll test in `test/Bitcode`.
>>>>> >
>>>>> > Moreover, the test file should be the `.ll` file that was used to
>>>>> > generate the bitcode.  Please add a comment that says what revision
>>>>> of
>>>>> > LLVM was used to generate the bitcode, something like:
>>>>> >
>>>>> >    ; Bitcode generated from llvm-as @ r123456.
>>>>> >
>>>>> >> +The input uses the older form without a dwoId field.
>>>>> >> +This should default to 0,
>>>>> >> +which is not displayed at all in the textual representation.
>>>>> >> +CHECK: !DICompileUnit
>>>>> >> +CHECK-NOT: dwoId:
>>>>> >
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150506/f9ee5398/attachment.html>