[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

So for the test case that this fixes I get:

  x i 8
  f1 b15 12 (DWARF 0x0)
  f2 b4 13 (DWARF 0x7)
  f3 b4 14 (DWARF 0x3)

For the 2/4/4 bitfield case I get:

  x i 8
  f1 b2 12 (DWARF 0x0)
  f2 b4 12 (DWARF 0x2)
  f3 b4 12 (DWARF 0x6)

Anyway, Adrian's and Jim's point seem right. DWARF contains the bit offset from the start of the 'container' (it seems ObjC just groups bitfields together by some algorithm). If I look at the code for printing bitfields it seems like we expect to get the *bit* offset from the layout we store and we get the *byte* offset from the runtime. And then we combine those two into the actual offset we have to read. The values we get from the example above seem to match that and get the expected result, so I think that's right.

Regarding the second test: Running the test (You need to add the `test_` prefix first) I get a pretty long warning that we try to get the size of the interface without an execution context:

  warning: trying to determine the size of type @interface HasBitfield2 : NSObject{
      unsigned int x;
      unsigned int field1;
      unsigned int field2;
      unsigned int field3;
  }
  @end
  without a valid ExecutionContext. this is not reliable. please file a bug against LLDB.
  backtrace:
  0  liblldb.11.0.0git.dylib 0x00000001070146b5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
  1  liblldb.11.0.0git.dylib 0x0000000106f16a1b lldb_private::TypeSystemClang::GetBitSize(void*, lldb_private::ExecutionContextScope*) + 827
  2  liblldb.11.0.0git.dylib 0x0000000106ba0421 lldb_private::CompilerType::GetByteSize(lldb_private::ExecutionContextScope*) const + 33
  3  liblldb.11.0.0git.dylib 0x00000001079bcd68 IRForTarget::CreateResultVariable(llvm::Function&) + 2760
  4  liblldb.11.0.0git.dylib 0x00000001079c30d0 IRForTarget::runOnModule(llvm::Module&) + 928
  5  liblldb.11.0.0git.dylib 0x00000001079a15a4 lldb_private::ClangExpressionParser::PrepareForExecution(unsigned long long&, unsigned long long&, std::__1::shared_ptr<lldb_private::IRExecutionUnit>&, lldb_private::ExecutionContext&, bool&, lldb_private::ExecutionPolicy) + 1572
  6  liblldb.11.0.0git.dylib 0x00000001079b723a lldb_private::ClangUserExpression::Parse(lldb_private::DiagnosticManager&, lldb_private::ExecutionContext&, lldb_private::ExecutionPolicy, bool, bool) + 1002
  7  liblldb.11.0.0git.dylib 0x0000000106b31e77 lldb_private::UserExpression::Evaluate(lldb_private::ExecutionContext&, lldb_private::EvaluateExpressionOptions const&, llvm::StringRef, llvm::StringRef, std::__1::shared_ptr<lldb_private::ValueObject>&, lldb_private::Status&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, lldb_private::ValueObject*) + 2375
  8  liblldb.11.0.0git.dylib 0x0000000106c358e5 lldb_private::Target::EvaluateExpression(llvm::StringRef, lldb_private::ExecutionContextScope*, std::__1::shared_ptr<lldb_private::ValueObject>&, lldb_private::EvaluateExpressionOptions const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, lldb_private::ValueObject*) + 885

In general the semantics here seem kinda off. The result of the expression is  `HasBitfield2` which I don't think is something we really have in Obj-C as it seems like a stack-allocated `HasBitfields2` instance. Changing the expression to return `HasBitfield2 *` instead and then forcing that we dereference Obj-C pointers fixes the second case for me. (FWIW, doing `frame var *hb2` is also working and is doing essentially the same thing as it just prints the children of the pointer). Fixing that can just be a follow up PR.

So I would say removing the sanity check here seems fine. We maybe want to have a specific Obj-C sanity check but I'm not even sure how this would look like. Maybe figuring out what's the maximum 'container size' we can have in Obj-C and checking that offset + size is below that makes sense, but otherwise I don't think we can do a lot. So I think my previous comment that this is ready to go beside some minor changes is IMHO the way to go.



================
Comment at: lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py:24
+    @expectedFailureAll()
+    def ExpressionOnObject(self):
+        self.build()
----------------
This method isn't a test at the moment as it doesn't have a `test` prefix in the name. E.g. `test_ExpressionOnObject` is the way to make this a test that actually runs. Currently it's just a normal method that is expected to be run by other `test_` methods.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83433/new/

https://reviews.llvm.org/D83433