[PATCH] First set of patches for type uniquing

[Manman Ren]

Hi Eric,

Thanks for reviewing the patch.


On Thu, Aug 22, 2013 at 12:48 PM, Eric Christopher <echristo at gmail.com>wrote:

> Hi Manman,
>
> I'm still looking at the patch but one comment that will affect a lot
> of the text of the patch is that I think the unique name should be
> passed into the type creation functions, not created inside the
> backend. This means that each front end can use the definition of
> "unique" that most appeals to them.
>


I know dragonegg calls those type creation functions, and there are maybe
other out-of-tree usages.
This patch provides default value for the unique name (an empty string) to
the type creation functions, so
it is not going to break dragonegg or out-of-tree projects. Inside the type
creation functions, if the passed-in
unique name is empty, DIBuilder will generate a unique name. I am not sure
if it is necessary for dragonegg
to have its own implementation of generating the unique name, and how hard
it is.

The patch uses mangled name for C++ in clang, and it falls back to
DIBuilder to provide a unique name for
other languages. I am not sure whether front end can provide a better name
than DIBuilder, so instead of
requiring each front end to have an implementation for generating unique
name, I choose to have a centralized
implementation in DIBuilder. That decision of course is debatable.

In summary, the patch does the following in terms of name generation:

>The clang frontend has two helper functions: getUniqueTagTypeName and
> getUniqueTypeName
>           The first one handles TagType and it uses ODR (One Definition
> Rule) for external c++ types. It is currently called for struct, class,
> union and enum.
>           The second one can be used for general C++ types, it uses
mangled name
> appended with CU's directory and file name to provide uniqueness across
CUs.
> It is currently used to generate unique name for subroutine type.
> For other types, DIBuilder will provide a unique name. DIBuilder also has
a helper
> function getUniqueTypeName, this is called when clients of DIBuilder do
not provide a unique name.
> Note that Clang can handle more types in
> follow-on patches if it can provide a better name than DIBuilder.


> Also I'm not sure why you've got an actual hashing algorithm in there.
> Can you explain?
>

Which hashing algorithm are you referring to?
I don't recall a hashing algorithm in the patch.


>
> Couple other things I noticed:
>
> +  // If the consumer of DIBuilder does not provide a unique name,
> generate one
> +  // here.
> +  MDString *UniqueStr = UniqueName.empty() ?
> +                        getUniqueTypeName(dwarf::DW_TAG_structure_type,
> Name) :
> +                        MDString::get(VMContext, UniqueName);
>
> I don't think we should do this. Have an example where we'd want to?
>

The patch has a default value of an empty string for the type creation
function, so if the caller of the type
creation function does not provide a name, DIBuilder will come up with one.


>
> +        Map.insert(std::make_pair(
> +                   std::make_pair(Ty.getUniqueName(), Ty.isForwardDecl()),
> +                   Ty));
>
> A pair of a pair is a bit icky here as well.
>
Any suggestion here?

Thanks,
Manman


>
> This should be enough to start discussion/revision of the patch.
>
> -eric
>
> On Wed, Jul 31, 2013 at 4:37 PM, Manman Ren <manman.ren at gmail.com> wrote:
> >
> > After cleaning up usage of the verify function and all the testing cases,
> > the first set of patches are ready for review :)
> >
> > 3 patches are included: one for clang (tu_clang_07312013.patch), one for
> > code change of llvm (tu_07312013.patch), and one for updating all the
> > testing cases (tu_test_07312013.patch).
> >
> > The following are implemented:
> > 1> DIType will have a unique name at field 9.
> >      For DIBasicType, DICompositeType, DIDerivedType, the fields after
> will
> > be shifted by 1.
> > 2> I am going to update usage of DIType in phases:
> >      At the first batch, getContainingType of DICompositeType and
> > DISubprogram will be replaced with a MDString, as well as getClassType of
> > DIDerivedType.
> > 3> The verifier is updated according to 1 and 2.
> > 4> DwarfDebug has a TypeMap.
> >       It also has a helper function lookupType to find the corresponding
> > DIType given a type name.
> > 5> how to generate the unique name?
> >       The clang frontend has two helper functions: getUniqueTagTypeName
> and
> > getUniqueTypeName
> >           The first one handles TagType and it uses ODR (One Definition
> > Rule) for external c++ types. It is currently called for struct, class,
> > union and enum.
> >           The second one can be used for general types, it uses mangled
> name
> > appended with CU's directory and file name to provide uniqueness across
> CUs.
> > It is currently used to generate unique name for subroutine type. For
> other
> > types, DIBuilder will provide a unique name. Clang can handle more types
> in
> > follow-on patches if it can provide a better name than DIBuilder.
> >       DIBuilder also has a helper function getUniqueTypeName, this is
> called
> > when consumer of DIBuilder does not provide a unique type name. This is
> > needed for dragonegg, before it provides a unique name (not sure whether
> it
> > is needed).
> > 6> All DITypes created by DIBuilder will be added to RetainTypes.
> >
> > Comments and feedback are welcome.
> >
> > Manman
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130822/486f3123/attachment.html>