Page MenuHomePhabricator

Fix how ValueObjectChild handles bit-fields stored in a Scalar in UpdateValue()
ClosedPublic

Authored by shafik on Aug 5 2020, 4:59 PM.

Details

Summary

When bit-field data was stored in a Scalar in ValueObjectChild during UpdateValue() it was extracting the bit-field value. Later on in lldb_private::DumpDataExtractor(…) we were again attempting to extract the bit-field:

s->Printf("%" PRIu64,
            DE.GetMaxU64Bitfield(&offset, item_byte_size, item_bit_size,
                                 item_bit_offset));

which would then not obtain the correct value. This will remove the extra extraction in UpdateValue().

We hit this specific case when values are passed in registers, which we could only reproduce in an optimized build.

Diff Detail

Event Timeline

shafik requested review of this revision.Aug 5 2020, 4:59 PM
shafik created this revision.
shafik added a comment.Aug 5 2020, 5:02 PM

Note: for the test I did not want to rely on clang choosing to pass the union by register, so I used assembly which ensures I will obtain the behavior I am looking to capture for the test.

friss added a subscriber: friss.Aug 5 2020, 5:37 PM
friss added inline comments.
lldb/source/Core/ValueObjectChild.cpp
202–205

Why remove the code in ValueObject rather than avoid re-extracting at printing time? I'm not sure which one is correct. If you get your hands on a ValueObject for the field in your test, what will GetValueAsUnsigned return? it should give the correct field value.

lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/TestValueObjPassByRef.py
10 ↗(On Diff #283288)

If the test in assembly is what we want, this is also architecture specific. It should be restricted to x86_64

davide requested changes to this revision.Aug 5 2020, 5:58 PM
davide added a subscriber: davide.
davide added inline comments.
lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/Makefile
2 ↗(On Diff #283288)

This is fundamentally a no-go. Depending on the optimization pipeline passes this in a register is an assumption that might go away at some point in the future and this test won't test what it has to & will still pass silently.

Something like this might work:
https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html

This revision now requires changes to proceed.Aug 5 2020, 5:58 PM
davide added inline comments.Aug 5 2020, 6:00 PM
lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/Makefile
2 ↗(On Diff #283288)

*depending on the optimization pipeline, the fact that is passed in a register is an assumption that

aprantl added inline comments.Aug 5 2020, 7:38 PM
lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/Makefile
1 ↗(On Diff #283288)

I think this is redundant. The default is a.out

2 ↗(On Diff #283288)

Given that the source code is a .s file, I think the -O1 is just redundant and can be removed. Using assembler is fine for this purpose.

8 ↗(On Diff #283288)

Is it possible to just override the rule that produces the .o file? Otherwise you are dropping the codesign and dsymutil phase.

lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/TestValueObjPassByRef.py
15 ↗(On Diff #283288)

lldbutil has a helper for running to a breakpoint by name.

shafik added inline comments.
lldb/source/Core/ValueObjectChild.cpp
202–205

lldb_private::DumpDataExtractor(…) is general purpose and it used by a lot of other code, it does know the value comes from a Scalar or otherwise it is just receiving a DataExtractor and obtaining the data from there.

lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/Makefile
2 ↗(On Diff #283288)

I did try using Specifying Registers for Local Variables and it did not work :-(

but in good news, I don't need the -O1 it was left over when I was struggling to get the test going.

2 ↗(On Diff #283288)

Yes, this was left over when I was experimenting trying to get the test to work I do not need the -O1.

8 ↗(On Diff #283288)

Let me see if I can remove the .o step.

lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/TestValueObjPassByRef.py
10 ↗(On Diff #283288)

Yes.

15 ↗(On Diff #283288)

I could not get it to work using lldbutil I was working w/ @JDevlieghere on this and he thought that made sense. IIUC I would have rewrite the assembly file to make it work.

shafik updated this revision to Diff 283647.Aug 6 2020, 9:59 AM

Removing use of -O1 from Makefile.

shafik updated this revision to Diff 283658.Aug 6 2020, 10:41 AM
  • Refactored the Makefile and test based on offline comments from Adrian.
shafik marked 7 inline comments as done.Aug 6 2020, 10:41 AM
friss added inline comments.Aug 6 2020, 10:44 AM
lldb/source/Core/ValueObjectChild.cpp
202–205

You didn't answer the most important question. Will GetValueAsUnsigned return the correct value on such a ValueObject once you remove this code?

shafik added inline comments.Aug 6 2020, 11:36 AM
lldb/source/Core/ValueObjectChild.cpp
202–205

apologies, misunderstood.

Yes, it does:

(lldb) script var = lldb.frame.FindVariable("u")
(lldb) script print(var.GetChildMemberWithName('raw'))
(uint32_t) raw = 1688469761
(lldb) script print(var.GetChildMemberWithName('a'))
(uint32_t:8) a = 1
(lldb) script print(var.GetChildMemberWithName('b'))
(uint32_t:8) b = 1
(lldb) script print(var.GetChildMemberWithName('c'))
(uint32_t:6) c = 36
(lldb) script print(var.GetChildMemberWithName('d'))
(uint32_t:2) d = 2
(lldb) script print(var.GetChildMemberWithName('e'))
(uint32_t:6) e = 36
(lldb) script print(var.GetChildMemberWithName('f'))
(uint32_t:2) f = 1
shafik added inline comments.Aug 6 2020, 12:21 PM
lldb/source/Core/ValueObjectChild.cpp
202–205

Whoops, copy-pasta:

(lldb) script print(var.GetChildMemberWithName('raw').GetValueAsUnsigned())
1688469761
(lldb) script print(var.GetChildMemberWithName('a').GetValueAsUnsigned())
1
(lldb) script print(var.GetChildMemberWithName('b').GetValueAsUnsigned())
1
(lldb) script print(var.GetChildMemberWithName('c').GetValueAsUnsigned())
36
(lldb) script print(var.GetChildMemberWithName('d').GetValueAsUnsigned())
2
(lldb) script print(var.GetChildMemberWithName('e').GetValueAsUnsigned())
36
(lldb) script print(var.GetChildMemberWithName('f').GetValueAsUnsigned())
1
shafik updated this revision to Diff 283703.Aug 6 2020, 12:26 PM
  • Add more tests
aprantl accepted this revision.Aug 6 2020, 12:31 PM

The test LGTM now! Please be sure to address Fred's comment before committing.

Do we have an end-to-end (with source code instead of assembler) test for ObjC bitfields, too? If not, it might still be a good a idea to add one even if it doesn't add coverage for this particular change.

I just have a small comment about the test. If you build the test with an llvm.org version of clang (best if it contains the git hash it was build from) and you don't include headers (they don't seem to be required for the test), then the file would be much easier to update/extend for other people.

shafik updated this revision to Diff 284507.Aug 10 2020, 2:49 PM

Updated to use llvm.org clang, remove header files from the main.c and add the commit hash for the clang so that it is easier to replicate the main.s in the future.

labath added a subscriber: labath.Aug 11 2020, 5:04 AM

This manifested itself for variables in registers because those end up being described as eValueTypeScalar, is that so?

If that's the case, then I think this would also manifest for variables described via DW_OP_constu 0xdead, DW_OP_stack_value. And if *that* is true, then this could be tested a lot simpler by having a global variable described by DW_OP_stack_value and "target variable"-ing it -- no makefiles, no python, just a simple .s file. And it would run on all platforms, not just darwin.

test/Shell/SymbolFile/DWARF/DW_OP_piece-struct.s tests a very similar thing in precisely this way. I suspect you could just take that test as a template and replace the struct definition with one that contains bitfieds.

This manifested itself for variables in registers because those end up being described as eValueTypeScalar, is that so?

If that's the case, then I think this would also manifest for variables described via DW_OP_constu 0xdead, DW_OP_stack_value. And if *that* is true, then this could be tested a lot simpler by having a global variable described by DW_OP_stack_value and "target variable"-ing it -- no makefiles, no python, just a simple .s file. And it would run on all platforms, not just darwin.

test/Shell/SymbolFile/DWARF/DW_OP_piece-struct.s tests a very similar thing in precisely this way. I suspect you could just take that test as a template and replace the struct definition with one that contains bitfieds.

This sounds like a great approach, I have unfortunately been struggling to get a test case that works. It looks like I am hitting another bug, I am not surprised b/c using slight variations on this code I have found other three other bugs with how we deal with DW_OP_piece, DW_OP_bit_piece and I think DW_AT_const_value respectively.

I have been working w/ this:

#include <stdio.h>

typedef union
{
  unsigned raw;
  struct 
  {
    unsigned a : 8;
    unsigned b : 8;
    unsigned c : 6;
    unsigned d : 2;
    unsigned e : 6;
    unsigned f : 2;
  } ; 
} U;

static U ug= (U)(unsigned)0x0;

void f(U u) {
  printf( "%d\n", u.raw);
  return;
}

int main() {
  ug.raw = 0x64A40101;

  f(ug);
  printf( "%d\n", ug.raw);
}

and compiling as using clang -O1 -gdwarf-4 but we obtain bad values:

(lldb) target variable ug
(U) ug = {
  raw = 3395301392
   = (a = 16, b = 48, c = 32, d = 1, e = 10, f = 3)
}

I tried generating assembly and manually adjusting the debug info but I was not able to get very far there.

FYI this was the DWARF expression it was generating:

DW_AT_location	(DW_OP_addr 0x100002010, DW_OP_deref_size 0x1, DW_OP_constu 0x64a40101, DW_OP_mul, DW_OP_lit0, DW_OP_plus, DW_OP_stack_value)

If you can come up w/ a way to generate a test that produces correct values I am happy to use it!

We found a way to hand modify the assembly and it looks good, I just need to convert it to a test.

shafik updated this revision to Diff 285176.Aug 12 2020, 1:47 PM

Replacing python test with Shell test

I'll leave the test review to Pavel who knows that much better, but I have two last nits about the test.

lldb/test/Shell/SymbolFile/DWARF/valueobject-pass-by-reg.s
59 ↗(On Diff #285176)

I think this was still generated with system clang. info_string below says this was compiled by Apple clang version 12.0.0 (clang-1200.0.31.1) and not the listed commit (which would create an info_string like clang version 12.0.0 (https://github.com/llvm/llvm-project 86dea1f39bd127776b999e10dff212003068d30a).)

149 ↗(On Diff #285176)

You can avoid these system-specific paths by compiling the file in /tmp with your cwd in /tmp and passing -isysroot / to the clang invocation. This way this section would look like this for everyone independently of their system username or macOS version (which will make updating this much easier):

.asciz  "clang version 12.0.0 (https://github.com/llvm/llvm-project 6acb897dfbc0ec22007cde50b3bc9c60f4674fb2)" ## string offset=0
.asciz  "/tmp/weird.c"                  ## string offset=101                  
.asciz  "/"                             ## string offset=114                  
.asciz  "/tmp"                          ## string offset=116                  
.asciz  "ug"                            ## string offset=121                  
.asciz  "U"                             ## string offset=124                  
.asciz  "raw"                           ## string offset=126                  
.asciz  "unsigned int"                  ## string offset=130                  
.asciz  "a"                             ## string offset=143 
friss added inline comments.Aug 12 2020, 4:06 PM
lldb/test/Shell/SymbolFile/DWARF/valueobject-pass-by-reg.s
14–40 ↗(On Diff #285176)

This gives a much more compact debug info section:

typedef union {
  unsigned raw;
  struct {
     unsigned a : 8;
     unsigned b : 8;
     unsigned c : 6;
     unsigned d : 2;
     unsigned e : 6;
     unsigned f : 2;
  };
} U;

// This appears first in the debug info and pulls the type definition in...
static U __attribute__((used)) _type_anchor;
// ... then our useful variable appears last in the debug info and we can
// tweak the assembly without needing to edit a lot of offsets by hand.
static U ug;

extern void f(U);

// Omit debug info for main.
__attribute__((nodebug))
int main() {
  ug.raw = 0x64A40101;
  f(ug);
  f((U)ug.raw);
}

You can easily edit out the TEXT section, the line table and the accelerator tables and patch the location expression to give you a minimal binary.

62–63 ↗(On Diff #285176)

I don't think you need the TEXT segment at all, or at least you can make it empty.

shafik updated this revision to Diff 285233.Aug 12 2020, 6:49 PM
shafik marked 4 inline comments as done.

Updated test using more compact code from Fred.

labath accepted this revision.Aug 13 2020, 5:45 AM

This looks good to me. I appreciate the efforts taken to reduce the test size.

The trick for controlling the debug info order is neat, and I may end up using it some time. FWIW, the way I usually handle these things is by first replacing all constant debug info offsets with symbolic references (.long 123 -> .long .Lmytype-.debug_info). After that, it's possible to freely manipulate any debug info entry. (At that point I usually delete all DW_AT_decl_file/lines and other boring attributes, which tends to reduce the file a lot).

lldb/test/Shell/SymbolFile/DWARF/valueobject-pass-by-reg.s
1 ↗(On Diff #285233)

I don't think the test file name matches what is being tested anymore. Maybe name it something like DW_AT_data_bit_offset-DW_OP_stack_value (in line with other tests in this folder)?

shafik updated this revision to Diff 285423.Aug 13 2020, 10:31 AM
shafik marked an inline comment as done.

Update test name

This revision was not accepted when it landed; it landed in state Needs Review.Aug 13 2020, 11:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2020, 11:53 AM