[lldb-dev] r247953 - TypeSystem is now a plugin interface

[Zachary Turner via lldb-dev]

Well my point is just that clang already has hundreds of tests for this
type of thing, but they don't test what happens in the presence of an
external record layout, which is what LLDB uses.  And it tests far more
scenarios than we could ever think of.  So I don't think we need to write
many tests for this in LLDB, we just need to verify that there are no bugs
in clang, because right now it has no test coverage.

On Mon, Oct 5, 2015 at 12:52 PM Greg Clayton <clayborg at gmail.com> wrote:

> You can just create types in a multiline expression and then use them and
> verify things since we don't generate debug info for expressions by
> default. Should be very easy tests to write...
>
> > On Oct 5, 2015, at 12:50 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Yes, it was a compiler bug.  Funny that it worked before your change,
> but anyway I'm glad it got exposed.
> >
> > The reason this was never caught before is because clang has the notion
> of internal layout (clang compiles the program itself) vs. external record
> layout (clang uses layout info from an external source, like DWARF).
> There's hundreds of tests for internal layout, but almost 0 for external
> layout.  I added a task to my backlog to add a feature to clang that will
> allow every single internal record layout test to work with external record
> layout as well.  In theory this could find a ton of new bugs in clang for
> both Windows as well as other platforms that affect LLDB.
> >
> > On Mon, Oct 5, 2015 at 12:45 PM Greg Clayton <clayborg at gmail.com> wrote:
> > Back from vacation, sorry for the delay.
> >
> > Sounds like you fixed this. But just wanted to clarify a few things. We
> use the offsets into in the DWARF and we assist the compiler in laying out
> types. So as long as the compiler is correct, we will get things right.
> DWARF doesn't capture things like "#pragma pack(...)" or many other object
> level attributes or things that can affect class layout, so we assist the
> compiler when it lays out RecordDecl objects. The clang::ExternalASTSource
> has callbacks that can be implemented in order to layout virtual/non
> virtual base classes and instance variables.
> >
> > So we won't get things wrong as long as there isn't a compiler bug,
> which seemed to be your case.
> >
> > Greg
> >
> > > On Sep 29, 2015, at 3:39 PM, Zachary Turner <zturner at google.com>
> wrote:
> > >
> > > Hi Greg.  I'm responding on this thread since you said lldb-dev is a
> better place for questions.
> > >
> > > I had a chance to look at this some more. It looks like the way we're
> constructing the clang::RecordDecl is incorrect on Windows.  I'm not sure
> what record layout is like on non-Windows platforms, but if we have this
> code:
> > >
> > > class A {
> > >     virtual ~A() {}
> > >     int a;
> > > };
> > >
> > > class B : public A {
> > >     virtual ~B() {}
> > >     int b;
> > > };
> > >
> > > A * pa = new B ();
> > >
> > > then using the MS ABI, offset of a is 4 and offset of B is 8.  I
> verified this by modifying the source of sbvalue-cast.cpp to print the
> offsets.
> > >
> > > d:\src\llvm\tools\lldb\test\lang\cpp\dynamic-value>a.out
> > > DerivedA::ctor()->
> > > m_base_val=20
> > > m_a_val=10
> > > offset of DerivedA::m_a_val is 8
> > > offset of DerivedA::m_base_val is 4
> > >
> > > Clang agrees with me as well.  Compiling with -Xclang -emit-llvm-only
> -Xclang -fdump-record-layouts gives me the following AST Record Layout
> > >
> > > *** Dumping AST Record Layout
> > >    0 | class DerivedA
> > >    0 |   class Base (primary base)
> > >    0 |     (Base vftable pointer)
> > >    4 |     int m_base_val
> > >    8 |   int m_a_val
> > >      | [sizeof=12, align=4
> > >      |  nvsize=12, nvalign=4]
> > >
> > > On the LLDB side, we have ClangASTContext::GetChildCompilerTypeAtIndex
> which computes the byte offset, field name, etc for the ValueObject.  On
> line 5754 it begins querying the name and byte offset of the field.  The
> name returns `m_base_val`, but the field_offset returns 8 bytes, which we
> can see above is wrong.
> > >
> > > Any suggestions where I might look to dig further?
> > >
> > > On Wed, Sep 23, 2015 at 10:34 AM Greg Clayton <clayborg at gmail.com>
> wrote:
> > > I responded in another e-mail with follow stuff you will need to send
> me. Always send questions to lldb-dev and not lldb-commits as there is so
> much traffic in lldb-commmits it might get lost. I keep up with lldb-dev
> daily.
> > >
> > > > On Sep 22, 2015, at 10:12 AM, Zachary Turner <zturner at google.com>
> wrote:
> > > >
> > > > In regards to this CL:
> http://llvm.org/viewvc/llvm-project?rev=247953&view=rev
> > > >
> > > > It has broken TestCppValueCast on Windows and fixed TestCxxWcharT on
> Windows as well.  Although these appear to be platform specific behavioral
> changes, as I don't see anyone else reporting failures or fixes, the
> description of the CL leads me to infer that this change was supposed to
> have no functional change.  I haven't gotten any response pinging the
> thread so I'll pose it to the wider list for visibility in case someone can
> shed some light on this.  In particular, is this supposed to come with a
> functional change, and if so what?
> > > >
> > > > Unfortunately I don't have buildbots running tests on Windows yet,
> so I can't justify reverting the CL (and it's too old to revert now anyway
> since other things depend on it), but it would greatly appreciate if
> someone can take a look at this and see if they can identify the source of
> the behavioral change
> > > >
> > > > I do plan to have a buildbot running tests within a week or two, so
> hopefully we can catch this kind of thing much sooner next time.
> > >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20151005/e19c2c31/attachment.html>