shafik added a subscriber: JDevlieghere. shafik added inline comments.
================ Comment at: lldb/source/Core/ValueObjectChild.cpp:202-205 - if (m_bitfield_bit_size) - scalar.ExtractBitfield(m_bitfield_bit_size, - m_bitfield_bit_offset); - else ---------------- friss wrote: > 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_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. ================ Comment at: lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/Makefile:2 +EXE := a.out +CFLAGS := -O1 + ---------------- aprantl wrote: > davide wrote: > > davide wrote: > > > 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 > > *depending on the optimization pipeline, the fact that is passed in a > > register is an assumption that > 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. 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. ================ Comment at: lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/Makefile:2 +EXE := a.out +CFLAGS := -O1 + ---------------- shafik wrote: > aprantl wrote: > > davide wrote: > > > davide wrote: > > > > 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 > > > *depending on the optimization pipeline, the fact that is passed in a > > > register is an assumption that > > 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. > 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. Yes, this was left over when I was experimenting trying to get the test to work I do not need the `-O1`. ================ Comment at: lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/Makefile:8 + $(CC) $(CFLAGS) $(SRCDIR)/main.s -c -o main.o + $(CC) $(CFLAGS) main.o -o a.out ---------------- aprantl wrote: > Is it possible to just override the rule that produces the .o file? Otherwise > you are dropping the codesign and dsymutil phase. Let me see if I can remove the `.o` step. ================ Comment at: lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/TestValueObjPassByRef.py:10 + + @skipUnlessDarwin + def test(self): ---------------- friss wrote: > If the test in assembly is what we want, this is also architecture specific. > It should be restricted to x86_64 Yes. ================ Comment at: lldb/test/API/functionalities/data-formatter/valueobj-pass-by-reg/TestValueObjPassByRef.py:15 + self.runCmd("b f"); + self.runCmd("run"); + ---------------- aprantl wrote: > lldbutil has a helper for running to a breakpoint by name. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85376/new/ https://reviews.llvm.org/D85376 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
