[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: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).



> 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/8e4f7fcd/attachment.html>