<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 7, 2018 at 12:48 AM, Sebastian Huber <span dir="ltr"><<a href="mailto:sebastian.huber@embedded-brains.de" target="_blank">sebastian.huber@embedded-brains.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 06/08/18 21:14, Joel Sherrill wrote:<div><div class="gmail-h5"><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
On Mon, Aug 6, 2018 at 1:21 AM, Chris Johns <<a href="mailto:chrisj@rtems.org" target="_blank">chrisj@rtems.org</a> <mailto:<a href="mailto:chrisj@rtems.org" target="_blank">chrisj@rtems.org</a>>> wrote:<br>
<br>
On 06/08/2018 16:12, Christian Mauderer wrote:<br>
> Am 06.08.2018 um 07:31 schrieb Chris Johns:<br>
>> On 06/08/2018 10:51, Chris Johns wrote:<br>
>>> On 05/08/2018 19:39, Christian Mauderer wrote:<br>
>>>> Am 05.08.2018 um 04:00 schrieb Chris Johns:<br>
>>>>> Hi,<br>
>>>>><br>
>>>>> I have been working on migrating covoar in the rtems-tools<br>
repo to DWARF. The<br>
>>>>> goal is remove objdump parsing and to get accurate details<br>
about the functions<br>
>>>>> being covered. This is an unfunded task.<br>
>>>>><br>
>>>>> The work has resulted in a close examination of inlined code<br>
in RTEMS and what I<br>
>>>>> saw alarmed me so I have added a report to the rtems-exeinfo<br>
tool in rtems-tools<br>
>>>>> (the change is to be posted for review once I get the<br>
coverage tests running).<br>
>>>>><br>
>>>>> A summary report for hello.exe on RTEMS 5 for SPARC is:<br>
>>>>><br>
>>>>> inlined funcs : 1412<br>
>>>>> total funcs : 1956<br>
>>>>> % inline funcs : 72%<br>
>>>>> total size : 174616<br>
>>>>> inline size : 81668<br>
>>>>> % inline size : 46%<br>
>>>>><br>
>>>>> This is a small application so it could be argued that skews<br>
the figures. A<br>
>>>>> large C/C++ application built with -O2 running on RTEMS 4.11<br>
ARM reports the<br>
>>>>> inline usage as:<br>
>>>>><br>
>>>>> inlined funcs : 10370<br>
>>>>> total funcs : 17700<br>
>>>>> % inline funcs : 58%<br>
>>>>> total size : 3066240<br>
>>>>> inline size : 1249514<br>
>>>>> % inline size : 40%<br>
>>>>><br>
>>>>> This does not seem right to me.<br>
>>>>><br>
>>>>> The report is new and there could be issues in the DWARF<br>
handling that feeds<br>
>>>>> this report however I am posting this to start a discussion<br>
on the topic of<br>
>>>>> inlining.<br>
>>>>><br>
>>>>> I attach the report for hello.exe. The `-i` option generates<br>
the inline report.<br>
>>>>><br>
>>>>> The first section is a summary showing the total number of<br>
functions in the<br>
>>>>> executable that have machine code and are flagged as inline.<br>
The report lists<br>
>>>>> the percentage of functions that are inlined and the<br>
percentage of machine code<br>
>>>>> that is inlined. The values seem high to me.<br>
>>>>><br>
>>>>> The second table lists inline functions that are repeated<br>
sorted from the<br>
>>>>> largest foot print to the smallest. The first column the<br>
total size of machine<br>
>>>>> code in the executable and the second column the number of<br>
instances.<br>
>>>>><br>
>>>>> The third table is the list of inline functions sorted from<br>
largest machine code<br>
>>>>> footprint to smallest. The second column are flags of which<br>
there is one. A `E`<br>
>>>>> indicates the inline function is also external which means<br>
the compiler has<br>
>>>>> created an external reference to this function, ie an<br>
address-of is being taken.<br>
>>>>> The third column is the address in the executable so you can<br>
take a look with<br>
>>>>> objdump at the machine code.<br>
>>>>><br>
>>>>> We need to ask some important question in relation to<br>
inlining. It is cheap to<br>
>>>>> add and we all feel the code we add needs to be fast and<br>
needs to be inlined but<br>
>>>>> does it really need to be inlined?<br>
>>>>><br>
>>>>> Some pieces of code do need to be inlined and the overhead<br>
is just that an<br>
>>>>> overhead, for example in the large C/C++ application there<br>
is a low level<br>
>>>>> volatile hardware write routing with close to 300 instances<br>
and a code size of<br>
>>>>> 10K. This code needs to be inlined for performance reasons<br>
but should the size<br>
>>>>> on average be 40 bytes, I doubt it.<br>
>>>>><br>
>>>>> Can we be more judicious with our use of the inline keyword?<br>
>>>>><br>
>>>>> Is the performance gain we really expect or is the actual<br>
overhead of a call<br>
>>>>> frame not worth saving?<br>
>>>>><br>
>>>>> What are the real costs of inlining a piece of code? It adds<br>
size to the<br>
>>>>> executable and depending on the code being inlined it<br>
complicates coverage<br>
>>>>> analysis be adding extra branch points.<br>
>>>>><br>
>>>>> The metrics to determine what should be inlined is<br>
complicated and I do not<br>
>>>>> think we have a suitable policy in place. I believe it is<br>
time we to create one.<br>
>>>>><br>
>>>>> The issue is not limited to our code, gcc, newlib and<br>
libstdc++ seem to have<br>
>>>>> some code that should be looked at more closely. For example<br>
__udivmoddi4, and<br>
>>>>> __sprint_r.<br>
>>>>><br>
>>>>> Chris<br>
>>>>><br>
>>>>><br>
>>>><br>
>>>> Hello Chris,<br>
>>>><br>
>>>> I just took a look at one of the first function in your list:<br>
__sprint_r<br>
>>>><br>
>>>><br>
<a href="https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libc/stdio/vfprintf.c;h=c4bf2dbe31da64462ecccec97c8e901e4ffadd44;hb=HEAD#l403" rel="noreferrer" target="_blank">https://sourceware.org/git/git<wbr>web.cgi?p=newlib-cygwin.git;a=<wbr>blob;f=newlib/libc/stdio/<wbr>vfprintf.c;h=c4bf2dbe31da64462<wbr>ecccec97c8e901e4ffadd44;hb=<wbr>HEAD#l403</a><br>
<<a href="https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libc/stdio/vfprintf.c;h=c4bf2dbe31da64462ecccec97c8e901e4ffadd44;hb=HEAD#l403" rel="noreferrer" target="_blank">https://sourceware.org/git/gi<wbr>tweb.cgi?p=newlib-cygwin.git;<wbr>a=blob;f=newlib/libc/stdio/<wbr>vfprintf.c;h=c4bf2dbe31da64462<wbr>ecccec97c8e901e4ffadd44;hb=<wbr>HEAD#l403</a>><br>
>>>><br>
>>>> As far as I can see, there is no explicit inline key word for<br>
that<br>
>>>> function. So in that case, the compiler decided that it would<br>
be a good<br>
>>>> idea to inline that function.<br>
>>><br>
>>> Thanks and yes. At this point in time I cannot tell what is<br>
happening and I am<br>
>>> not sure the tool is reporting accurate data, I need to<br>
investigate.<br>
>><br>
>> I have updated the tool and report to show which inline<br>
functions are:<br>
>><br>
>> - inlined by compiler<br>
>> - declared inline and not inlined<br>
>> - declared inline and inlined<br>
>><br>
>> I have also fixed a quick hack I had where the size was the<br>
span from the low PC<br>
>> to the high PC, this was wrong. Inlined code can be split and<br>
moved when<br>
>> inlining creating a discontinuous address range. The size in<br>
the report is now<br>
>> the number of machine code bytes.<br>
>><br>
>> The report will show any functions not inlined when asked to be<br>
inlined. We do<br>
>> not have any.<br>
>><br>
>> The 'C' flag in the inlined table shows which functions the<br>
compiler has inlined.<br>
>><br>
>> Chris<br>
>><br>
><br>
> With that list it is now much clearer which functions would be<br>
relevant<br>
> for a potential review.<br>
<br>
Yes. FYI the C/C++ RTEMS 4.11 app now gives:<br>
<br>
inlined funcs : 10370<br>
total funcs : 17700<br>
% inline funcs : 58%<br>
total size : 2296354<br>
inline size : 479628<br>
% inline size : 20%<br>
<br>
This level of reduction is more inline with what I expected.<br>
<br>
><br>
>>><br>
>>>> I'm not sure whether I might just haven't seen it but is there a<br>
>>>> possibility to distinguish between functions that have been<br>
inlined by<br>
>>>> the compiler and ones that have been inlined due to the<br>
"inline" keyword<br>
>>>> without looking at every definition?<br>
>>><br>
>>> I am not sure. The DWARF data is complex and detailed and I<br>
view this initial<br>
>>> step into the area of using DWARF to perform static analysis<br>
of RTEMS<br>
>>> executables as green.<br>
>>><br>
>>> DWARF does provide declaration attributes. I need to review<br>
the DWARF data and<br>
>>> standard to determine if we can tell what is declared inline<br>
and what has been<br>
>>> inlined. I think it would be good to know.<br>
>>><br>
>>>> Did you try compiling with size optimization? I would expect<br>
that the<br>
>>>> compiler would inline far less functions and maybe even<br>
ignore some<br>
>>>> "inline" keywords. As far as I know it's more of a hint to<br>
the compiler.<br>
>>><br>
>>> Not yet. A complete tool build with those options is a lot of<br>
effort and I am<br>
>>> still not comfortable the report is accurate. I think this is<br>
something that<br>
>>> should be done at some point. I think it would create an<br>
interesting data point.<br>
>>><br>
>>>> I would only worry about functions that are still inlined if size<br>
>>>> optimization is selected.<br>
>>><br>
>>> I think we need to review the functions we currently have<br>
tagged as inline. I<br>
>>> think the only way we can do this is with real data.<br>
>>><br>
>>>> That's the case when I tell the compiler to<br>
>>>> make the program as small as possible. In all other cases I<br>
want some<br>
>>>> well balanced optimum between speed and size. Inlining small<br>
functions<br>
>>>> is OK in that case if you ask me.<br>
>>><br>
>>> How do you define this, ie what is the inline policy we use?<br>
>>> How do you audit this?<br>
><br>
> Both questions are not simple to answer. It is most likely a case by<br>
> case decision. I think there are roughly two reasons for inlining:<br>
><br>
> - The code is short enough that it is smaller if it is inlined<br>
compared<br>
> to a function call.<br>
><br>
> - There is some performance reason.<br>
><br>
> Anything else?<br>
<br>
Not for inlining rather for not inlining? I had a discussion with<br>
Joel last week<br>
about coverage and he said he had previously reviewed the inlines<br>
to reduce the<br>
branch counts because it complicates coverage. I would prefer he<br>
talks about<br>
this, it is his area.<br>
<br>
<br>
Thanks for the lead-in Chris.<br>
<br>
First, there are multiple types of coverage. Statement, Decision coverage,<br>
MC/DC, and object coverage. Based on various limiting factors which I<br>
am happy to discuss in more detail if someone is interested, we went<br>
with object coverage. I will briefly remind everyone of the limitations<br>
and guidelines we went into this with:<br>
<br>
+ We didn't want instrumented code. Test as you fly, fly as you test.<br>
Plus there would need to be a way to get information generated by<br>
the instrumentation off the target.<br>
<br>
+ Without gcc support for identifying sub-expressions in complex<br>
logical expressions, there is no way to do DC or MCDC. Further,<br>
we would have had to have a tool to interpret the non-existent<br>
information and know which entries in the truth tables for every<br>
expression had to be exercised for DC and MCDC.<br>
<br>
+ gcov's reports are primarily statement coverage. And you can't get<br>
gcov reports without gcov output from runs which we still are working<br>
on. Also in terms of DO-178, statement coverage is generally Level C<br>
type coverage.<br>
<br>
We wanted higher level coverage for RTEMS and thus object<br>
coverage with emphasis on every generated branch statement being<br>
needed and exercisable as taken and not taken. This approach<br>
ensured we didn't have dead code.<br>
<br>
Plus this type of information could easily be obtained from simulator<br>
traces from Qemu and tsim. covoar was written to combine and<br>
correlate the information back to assembly and source and generate<br>
the reports.<br>
<br>
That's what led to the current coverage approach. Chris has been<br>
working to eliminate the use of command line tools (e.g. nm, objdump,<br>
etc) in favor of using DWARF directly.<br>
<br>
That's the background of the approach and how we got reports.<br>
<br>
Also remember that RTEMS testing is primarily via the public APIs<br>
which combined with coverage ensures we actually need all the<br>
internal code. If you can't reach it from a public API, it is dead.<br>
<br>
But the insights into RTEMS coding practices began when we started<br>
trying to improve the coverage percentages. Key point:<br>
<br>
IF CODE IS DUPLICATED AT THE SOURCE OR OBJECT LEVEL, IT HAS TO<br>
BE FULLY TESTED AT EACH INSTANCE OR INSTANTIATION.<br>
<br>
At the source level, sometimes we factored out a common helper method<br>
to avoid testing the same logic via multiple APIs. This reduced the number<br>
of test cases needed.<br>
<br>
We looked at a lot of generated assembly. Sometimes we would see<br>
large methods being inlined multiple times. This would increase the overall<br>
size of an RTEMS application. But size is not the only impact of inlining.<br>
If an inlined method has one or more if's in it, then the branch paths<br>
it includes are introduced EVERYWHERE it is inlined. When we<br>
had _Thread_Dispatch_enable, I recall it was used > 100 times and<br>
includes a branch in it. There was a build option to not inline this<br>
routine to avoid needing to add over 100 test cases.<br>
<br>
There is a delicate code size and complexity dance when deciding<br>
whether to inline something or not. There isn't an easy or obvous<br>
answer.<br>
<br>
+ How large is the method?<br>
<br>
+ How many times is it used?<br>
<br>
+ How many branches/loops does it have (e.g. cyclomatic complexity)?<br>
<br>
In general, very small methods with no branches can be inlined in less<br>
code than the subroutine call takes. So these tend to be a win.<br>
<br>
Over a certain size, the number of times used multiplied by the inlined<br>
size can have a negative impact on code size. We saw this a lot when<br>
doing the code coverage improvements.<br>
<br>
If the inlined method has branches, is it possible or cost effective<br>
to write test cases for the branches introduced at EVERY use?<br>
<br>
I came away believing a few things:<br>
<br>
+ The MISRA rule for using simple, single condition branch logic<br>
is really good for code coverage analysis. It simplifies it. Statement<br>
coverage and branch coverage become closer to equivalent. The<br>
resulting assembly code is also much easier to match to source.<br>
<br>
+ Inlining a method with conditional logic has to be looked at<br>
very carefully. You don't want to inline something and find yourself<br>
having exploded the required number of test cases. Again, look<br>
at the number of times a method is used but be cautious about<br>
conditional expressions in inline methods.<br>
<br>
+ Inlining doesn't always improve performance measurably. It<br>
can increase code size, code to branch around, etc.<br>
<br>
For a current example of something that is very likely a problem<br>
from a object code test coverage and code size increase view,<br>
consider _Thread_Dispatch_enable. It is inlined 41 times.<br>
Chris and I look<a href="https://maps.google.com/?q=ed+at+a+place+where+it+was+inlined.+It+w&entry=gmail&source=g">ed at a place where it was inlined. It w</a>as about<br>
80 bytes. 41*80 ==> 3280 bytes. Based on size along, it would<br>
save ~3K across the entire RTEMS API object code to not inline it<br>
at all.<br>
<br>
Then I looked at the cyclomatic complexity of _Thread_Dispatch_enable().<br>
>From Wikipedia:<br>
<br>
Cyclomatic complexity is a software metric, used to indicate the complexity<br>
of a program. It is a quantitative measure of the number of linearly independent<br>
paths through a program's source code.<br>
<br>
I used <a href="http://www.lizard.ws/" rel="noreferrer" target="_blank">http://www.lizard.ws/</a> [1] to analyse this method. If you take the conditional<br>
compilation out of the code, the cyclomatic complexity is 4. This means that<br>
every place this is inlined, it introduces 3 extra paths to test. 41*3 ==> 123.<br>
<br>
My recommendation would be to evaluate the impact of not inlining this<br>
method at all. The negative impact on testing and code size are pretty high.<br>
<br>
Unfortunately, what I wrote up is just the guidelines. Things change over<br>
time. An inlined method that is a nice encapsulation when written could<br>
turn out to be too large to be inlined if 10 other use cases get added.<br>
<br>
I lean to a method can be inlined if it is small and no more than one<br>
branch statement in assembly. You can be more lenient if used only<br>
once or twice. But lean to NO branches on an inlined method if the<br>
number of uses is high.<br>
<br>
Ideally the inlined code is no larger than the code for the call itself<br>
and there are no branches. But going above that is not a reason to<br>
not inline.<br>
<br>
I know this was long but there are a lot of trade-offs. We want RTEMS to<br>
be as small as possible and as testable as possible. We don't have an<br>
army of people to write test cases. And I don't want to need that.<br>
<br>
Also I have focused on the things that the RTEMS community has control<br>
over. Chris' tool gives us a broader view and we can likely help improve<br>
things more broadly.<br>
<br>
[1] You can cut and paste code into this via the web. So pretty easy to<br>
look at a single method.<br>
</blockquote>
<br></div></div>
I think all this information should be moved to a proper RTEMS contributors/engineering guide.</blockquote><div><br></div><div>+1 </div><div><br></div><div><a href="https://devel.rtems.org/wiki/TBR/UserManual/RTEMS_Coverage_Analysis">https://devel.rtems.org/wiki/TBR/UserManual/RTEMS_Coverage_Analysis</a> probably</div><div>needs scrubbing and merged into a Rest document.</div><div><br></div><div>It has some good stuff but some of the procedural stuff is out of date.</div><div><br></div><div>--joel</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-im gmail-HOEnZb"><br>
<br>
-- <br>
Sebastian Huber, embedded brains GmbH<br>
<br>
Address : Dornierstr. 4, D-82178 Puchheim, Germany<br>
Phone : +49 89 189 47 41-16<br>
Fax : +49 89 189 47 41-09<br>
E-Mail : <a href="mailto:sebastian.huber@embedded-brains.de" target="_blank">sebastian.huber@embedded-brain<wbr>s.de</a><br></span><div class="gmail-HOEnZb"><div class="gmail-h5">
PGP : Public key available on request.<br>
<br>
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.<br>
<br>
</div></div></blockquote></div><br></div></div>