<div dir="ltr">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.<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 5, 2015 at 12:52 PM Greg Clayton <<a href="mailto:clayborg@gmail.com">clayborg@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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...<br>
<br>
> On Oct 5, 2015, at 12:50 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
> Yes, it was a compiler bug. Funny that it worked before your change, but anyway I'm glad it got exposed.<br>
><br>
> 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.<br>
><br>
> On Mon, Oct 5, 2015 at 12:45 PM Greg Clayton <<a href="mailto:clayborg@gmail.com" target="_blank">clayborg@gmail.com</a>> wrote:<br>
> Back from vacation, sorry for the delay.<br>
><br>
> 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.<br>
><br>
> So we won't get things wrong as long as there isn't a compiler bug, which seemed to be your case.<br>
><br>
> Greg<br>
><br>
> > On Sep 29, 2015, at 3:39 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> ><br>
> > Hi Greg. I'm responding on this thread since you said lldb-dev is a better place for questions.<br>
> ><br>
> > 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:<br>
> ><br>
> > class A {<br>
> > virtual ~A() {}<br>
> > int a;<br>
> > };<br>
> ><br>
> > class B : public A {<br>
> > virtual ~B() {}<br>
> > int b;<br>
> > };<br>
> ><br>
> > A * pa = new B ();<br>
> ><br>
> > 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.<br>
> ><br>
> > d:\src\llvm\tools\lldb\test\lang\cpp\dynamic-value>a.out<br>
> > DerivedA::ctor()-><br>
> > m_base_val=20<br>
> > m_a_val=10<br>
> > offset of DerivedA::m_a_val is 8<br>
> > offset of DerivedA::m_base_val is 4<br>
> ><br>
> > Clang agrees with me as well. Compiling with -Xclang -emit-llvm-only -Xclang -fdump-record-layouts gives me the following AST Record Layout<br>
> ><br>
> > *** Dumping AST Record Layout<br>
> > 0 | class DerivedA<br>
> > 0 | class Base (primary base)<br>
> > 0 | (Base vftable pointer)<br>
> > 4 | int m_base_val<br>
> > 8 | int m_a_val<br>
> > | [sizeof=12, align=4<br>
> > | nvsize=12, nvalign=4]<br>
> ><br>
> > 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.<br>
> ><br>
> > Any suggestions where I might look to dig further?<br>
> ><br>
> > On Wed, Sep 23, 2015 at 10:34 AM Greg Clayton <<a href="mailto:clayborg@gmail.com" target="_blank">clayborg@gmail.com</a>> wrote:<br>
> > 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.<br>
> ><br>
> > > On Sep 22, 2015, at 10:12 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> > ><br>
> > > In regards to this CL: <a href="http://llvm.org/viewvc/llvm-project?rev=247953&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=247953&view=rev</a><br>
> > ><br>
> > > 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?<br>
> > ><br>
> > > 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<br>
> > ><br>
> > > 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.<br>
> ><br>
><br>
<br>
</blockquote></div></div>