[lldb-dev] LLDB Evolution
Zachary Turner via lldb-dev
lldb-dev at lists.llvm.org
Fri Aug 26 18:19:33 PDT 2016
Yea, all I did was copy the code and reduce the indent level. There are
other issues with the code that are non-conformant for LLVM style, but
clang-format can catch all of those. It can't early indent your code for
you :)
On Fri, Aug 26, 2016 at 6:17 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
> On Aug 26, 2016, at 6:12 PM, Zachary Turner via lldb-dev <
> lldb-dev at lists.llvm.org> wrote:
>
> Back to the formatting issue, there's a lot of code that's going to look
> bad after the reformat, because we have some DEEPLY indented code. LLVM
> has adopted the early return model for this reason. A huge amount of our
> deeply nested code could be solved by using early returns. For example,
> here's some code in a function I was just looking at:
>
> // The 'A' packet is the most over designed packet ever here with
> // redundant argument indexes, redundant argument lengths and needed
> hex
> // encoded argument string values. Really all that is needed is a comma
> // separated hex encoded argument value list, but we will stay true to
> the
> // documented version of the 'A' packet here...
>
> Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
> int actual_arg_index = 0;
>
> packet.SetFilePos(1); // Skip the 'A'
> bool success = true;
> while (success && packet.GetBytesLeft() > 0)
> {
> // Decode the decimal argument string length. This length is the
> // number of hex nibbles in the argument string value.
> const uint32_t arg_len = packet.GetU32(UINT32_MAX);
> if (arg_len == UINT32_MAX)
> success = false;
> else
> {
> // Make sure the argument hex string length is followed by a
> comma
> if (packet.GetChar() != ',')
> success = false;
> else
> {
> // Decode the argument index. We ignore this really because
> // who would really send down the arguments in a random
> order???
> const uint32_t arg_idx = packet.GetU32(UINT32_MAX);
> if (arg_idx == UINT32_MAX)
> success = false;
> else
> {
> // Make sure the argument index is followed by a comma
> if (packet.GetChar() != ',')
> success = false;
> else
> {
> // Decode the argument string value from hex bytes
> // back into a UTF8 string and make sure the length
> // matches the one supplied in the packet
> std::string arg;
> if (packet.GetHexByteStringFixedLength(arg,
> arg_len) != (arg_len / 2))
> success = false;
> else
> {
> // If there are any bytes left
> if (packet.GetBytesLeft())
> {
> if (packet.GetChar() != ',')
> success = false;
> }
>
> if (success)
> {
> if (arg_idx == 0)
>
> m_process_launch_info.GetExecutableFile().SetFile(arg.c_str(), false);
>
> m_process_launch_info.GetArguments().AppendArgument(arg.c_str());
> if (log)
> log->Printf ("LLGSPacketHandler::%s
> added arg %d: \"%s\"", __FUNCTION__, actual_arg_index, arg.c_str ());
> ++actual_arg_index;
> }
> }
> }
> }
> }
> }
> }
>
>
>
> Whether we like early return or not, it is the LLVM Style, and when you
> have to deal with an 80 column wrapping limitation, it definitely helps.
> For example, the above function becomes:
>
>
> Since you’re going with the LLVM style, just a few notes (maybe you
> quickly added the early return without clang-formatting):
>
>
> // The 'A' packet is the most over designed packet ever here with
> // redundant argument indexes, redundant argument lengths and needed
> hex
> // encoded argument string values. Really all that is needed is a comma
> // separated hex encoded argument value list, but we will stay true to
> the
> // documented version of the 'A' packet here...
>
> Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
> int actual_arg_index = 0;
>
> packet.SetFilePos(1); // Skip the 'A'
> while (packet.GetBytesLeft() > 0)
> {
>
>
> Actually I believe the opening bracket here should be on the same line as
> the while.
>
> // Decode the decimal argument string length. This length is the
> // number of hex nibbles in the argument string value.
> const uint32_t arg_len = packet.GetU32(UINT32_MAX);
> if (arg_len == UINT32_MAX)
>
>
> Any reason the if isn’t on the same indentation as the previous line? (As
> for almost every other if apparently)
>
> return false;
> // Make sure the argument hex string length is followed by a comma
> if (packet.GetChar() != ',')
> return false;
>
> // Decode the argument index. We ignore this really because
> // who would really send down the arguments in a random order???
> const uint32_t arg_idx = packet.GetU32(UINT32_MAX);
> if (arg_idx == UINT32_MAX)
> return false;
>
> // Make sure the argument index is followed by a comma
> if (packet.GetChar() != ',')
> return false;
> // Decode the argument string value from hex bytes
> // back into a UTF8 string and make sure the length
> // matches the one supplied in the packet
> std::string arg;
> if (packet.GetHexByteStringFixedLength(arg, arg_len) != (arg_len / 2))
> return false;
> // If there are any bytes left
> if (packet.GetBytesLeft())
> {
> if (packet.GetChar() != ',')
> return false;
> }
>
>
> No brackets.
>
>
> if (arg_idx == 0)
> m_process_launch_info.GetExecutableFile().SetFile(arg.c_str(),
> false);
> m_process_launch_info.GetArguments().AppendArgument(arg.c_str());
> if (log)
> log->Printf ("LLGSPacketHandler::%s added arg %d: \"%s\"",
> __FUNCTION__, actual_arg_index, arg.c_str ());
> ++actual_arg_index;
> }
>
> This saves 24 columns!, which is 30% of the usable screen space in an
> 80-column environment.
>
>
> That’s a great motivating example to illustrate the “early return”
> motivation in llvm.
>
> —
> Mehdi
>
>
>
> On Thu, Aug 25, 2016 at 1:05 PM Zachary Turner <zturner at google.com> wrote:
>
>> Definitely agree we can't map everything to that model. I can imagine a
>> first step towards lit being where all our tests are literally exactly the
>> same as they are today, with Makefiles and all, but where lit is used
>> purely to recurse the directory tree, run things in multiple processes, and
>> spawn workers.
>>
>> Lit should be flexible enough to support this.
>>
>> As a further step, imagine rewriting most tests as inline tests like
>> lldbinline. Lit can support this too, the parsing and file commands are all
>> up to the implementation. So the existing model of run tool and grep output
>> is just one implementation of that, but you could design one that has build
>> commands, c++ source, and Python all in one file, or even intermingled like
>> in an lldbinline test
>>
>>
>> On Thu, Aug 25, 2016 at 12:54 PM Todd Fiala <todd.fiala at gmail.com> wrote:
>>
>>> On Mon, Aug 8, 2016 at 2:57 PM, Zachary Turner via lldb-dev <
>>> lldb-dev at lists.llvm.org> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Aug 8, 2016 at 2:40 PM Kate Stone via lldb-dev <
>>>> lldb-dev at lists.llvm.org> wrote:
>>>>
>>>>> LLDB has come a long way since the project was first announced. As a
>>>>> robust debugger for C-family languages and Swift, LLDB is constantly in use
>>>>> by millions of developers. It has also become a foundation for bringing up
>>>>> debugger support for other languages like Go and RenderScript. In addition
>>>>> to the original macOS implementation the Linux LLDB port is in active use
>>>>> and Windows support has made significant strides. IDE and editor
>>>>> integration via both SB APIs and MI have made LLDB available to even more
>>>>> users. It’s definitely a project every contributor can be proud of and I’d
>>>>> like to take a moment to thank everyone who has been involved in one way or
>>>>> another.
>>>>>
>>>>> It’s also a project that shows some signs of strain due to its rapid
>>>>> growth. We’ve accumulated some technical debt that must be paid off, and
>>>>> in general it seems like a good time to reflect on where we'll be headed
>>>>> next. We’ve outlined a few goals for discussion below as well as one more
>>>>> short-term action. Discussion is very much encouraged.
>>>>>
>>>>> *Forward-Looking Goals*
>>>>>
>>>>> 1. Testing Strategy Evaluation
>>>>>
>>>>> Keeping our code base healthy is next to impossible without a robust
>>>>> testing strategy. Our existing suite of tests is straightforward to run
>>>>> locally, and serves as a foundation for continuous integration. That said,
>>>>> it is definitely not exhaustive. Obvious priorities for improvement
>>>>> include gathering coverage information, investing in more conventional unit
>>>>> tests in addition to the suite of end-to-end tests, and introducing tests
>>>>> in code bases where we depend on debugger-specific behavior (e.g.: for
>>>>> expression evaluation.)
>>>>>
>>>>> I know this is going to be controversial, but I think we should at
>>>> least do a serious evaluation of whether using the lit infrastructure would
>>>> work for LLDB. Conventional wisdom is that it won't work because LLDB
>>>> tests are fundamentally different than LLVM tests. I actually completely
>>>> agree with the latter part. They are fundamentally different.
>>>>
>>>> However, we've seen some effort to move towards lldb inline tests, and
>>>> in a sense that's conceptually exactly what lit tests are. My concern is
>>>> that nobody with experience working on LLDB has a sufficient understanding
>>>> of what lit is capable of to really figure this out.
>>>>
>>>> I know when I mentioned this some months ago Jonathan Roelofs chimed in
>>>> and said that he believes lit is extensible enough to support LLDB's use
>>>> case. The argument -- if I remember it correctly -- is that the
>>>> traditional view of what a lit test (i.e. a sequence of commands that
>>>> checks the output of a program against expected output) is one particular
>>>> implementation of a lit-style test. But that you can make your own which
>>>> do whatever you want.
>>>>
>>>>
>>> I have some interest in looking into this. Kate and I had talked about
>>> it as a possible investigation back a few months ago. I'd be happy if we
>>> could reduce the overall complexity of building high quality tests. I'd be
>>> particularly interested in learning more about the alternative workflow
>>> that isn't just "run/check/run/check", as I don't think we can map
>>> everything to that model. I may take an action item on this in the near
>>> future.
>>>
>>>
>>>> This would not just be busy work either. I think everyone involved
>>>> with LLDB has experienced flakiness in the test suite. Sometimes it's
>>>> flakiness in LLDB itself, but sometimes it is flakiness in the test
>>>> infrastructure. It would be nice to completely eliminate one source of
>>>> flakiness.
>>>>
>>>> I think it would be worth having some LLDB experts sit down in person
>>>> with some lit experts and brainstorm ways to make LLDB use lit.
>>>>
>>>> Certainly it's worth a serious look, even if nothing comes of it.
>>>>
>>>>
>>>>
>>>>> 4. Good Citizenship in the LLVM Community
>>>>>
>>>>> Last, but definitely not least, LLDB should endeavor to be a good
>>>>> citizen of the LLVM community. We should encourage developers to think of
>>>>> the technology stack as a coherent effort, where common code should be
>>>>> introduced at an appropriate level in the stack. Opportunities to factor
>>>>> reusable aspects of the LLDB code base up the stack into LLVM will be
>>>>> pursued.
>>>>>
>>>>> One arbitrary source of inconsistency at present is LLDB’s coding
>>>>> standard. That brings us to…
>>>>>
>>>>> *Near-Term Goal: Standardizing on LLVM-style clang-format Rules*
>>>>>
>>>>> We’ve heard from several in the community that would prefer to have a
>>>>> single code formatting style to further unify the two code bases. Using
>>>>> clang-format with the default LLVM conventions would simplify code
>>>>> migration, editor configuration, and coding habits for developers who work
>>>>> in multiple LLVM projects. There are non-trivial implications to
>>>>> reformatting a code base with this much history. It can obfuscate history
>>>>> and impact downstream projects by complicating merges. Ideally, it should
>>>>> be done once with as much advance notice as is practical. Here’s the
>>>>> timeline we’re proposing:
>>>>>
>>>>> *Today* - mechanical reformatting proposed, comment period begins
>>>>>
>>>>> To get a preview of what straightforward reformatting of the code
>>>>> looks like, just follow these steps to get a clean copy of the repository
>>>>> and reformat it:
>>>>>
>>>>>
>>>>> 1. Check out a clean copy of the existing repository
>>>>> 2. Edit .clang-format in the root of the tree, remove all but the
>>>>> line “BasedOnStyle: LLVM”
>>>>> 3. Change your current working directory to the root of the tree
>>>>> to reformat
>>>>> 4. Double-check to make sure you did step 3 ;-)
>>>>> 5. Run the following shell command: "find . -name "*.[c,cpp,h]
>>>>> -exec clang-format -i {} +"
>>>>>
>>>>> Very excited about this one, personally. While I have my share of
>>>> qualms with LLVM's style, the benefit of having consistency is hard to
>>>> overstate. It greatly reduces the effort to switch between codebases, a
>>>> direct consequence of which is that it encourages people with LLVM
>>>> expertise to jump into the LLDB codebase, which hopefully can help to tear
>>>> down the invisible wall between the two.
>>>>
>>>> As a personal aside, this allows me to go back to my normal workflow of
>>>> having 3 edit source files opened simultaneously and tiled horizontally,
>>>> which is very nice.
>>>>
>>>>
>>>> _______________________________________________
>>>> lldb-dev mailing list
>>>> lldb-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>>>
>>>>
>>>
>>>
>>> --
>>> -Todd
>>>
>> _______________________________________________
> lldb-dev mailing list
> lldb-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160827/eeae663c/attachment-0001.html>
More information about the lldb-dev
mailing list