[Click] Minimizing patch size

[Philip Prindeville]

On 1/9/11 6:42 PM, Eddie Kohler wrote:
> On 1/9/11 6:09 PM, Philip Prindeville wrote:
>> Are we sure that the "::" edit is still required? It seems to me that
>> recent compilers are able to handle "::" in asm(...) sequences without
>> modification.
>
> Since automatic header transformation is required anyway, we do :: along with the other stuff.

That's a big change, and I'm thinking about smaller steps we could do in the meantime.

>
>> Can we move to a minimum compiler version that supports this?
>
> 0. Probably not.
> 1. I think asm(...::...) support was added quite recently and people compile Click with old compilers on embedded targets.

Not always.  OpenWRT is up to gcc 4.5.1.

And sometimes, you have to move to the newer compilers to get support for the enhanced instruction sets or the appropriate scheduling for current stepping rates.



> 2. Given automatic header transformation why would you care?

Plan B, in case one is needed.

> 3. If you have a specific patch or correct version number do send it for further discussion.

I'll try to figure out what version of gcc the fix appeared in.


>
> Eddie
>
>
>> Thanks.
>>
>> -Philip
>>
>> On 1/9/11 10:38 AM, Eddie Kohler wrote:
>>> Philip, just to speak up.
>>>
>>> The most important outstanding project for Click is exactly minimizing
>>> patch size. Any help is appreciated!
>>>
>>> The first step is to correctly understand the problem.
>>>
>>> > I still don't get this. Even in a .cpp file, you can have
>>> >
>>> > c++ stuff
>>> > ...
>>> > extern "C" {
>>> > #include "c-header-with-double-colon-asms.h"
>>> > }
>>> > ...
>>> >
>>> > and the compiler should be fine with that.
>>> >
>>> > If not, then the compiler is broken.
>>>
>>> Not quite. As Joonwoo points out "extern "C" {}" doesn't tell the C++
>>> compiler to stop being a C++ compiler. What is between the braces must
>>> still be correct C++: no use of C++ keywords as variables, no use of
>>> C++ operators like ::, and so forth. This is the main challenge of
>>> Click patching. Since Click's kernel module integrates with Linux, it
>>> must include Linux's header files; and therefore we must change those
>>> header files to be equivalent, but valid, C++ header files.
>>>
>>> Our patches have used a semi-mechanical process to make a bunch of
>>> syntactic changes (like :: => : : in asm statements), and then as you
>>> point out include 1000 lines of real changes.
>>>
>>> The way forward is "patchless". We have written a perl script that
>>> automates the semi-mechanical part. The script,
>>> linuxmodule/fixincludes.pl, generates new, C++-includable header files
>>> as part of the build process. These header files are equivalent to the
>>> Linux header files but safe for C++ compilers we care about.
>>>
>>> Unfortunately fixincludes.pl is not totally trivial and we have had to
>>> update it for new kernel releases. You can look at it to see what it
>>> does. Patches welcome.
>>>
>>> The goal of the patchless project is to make a version of Click that
>>> can be loaded into an UNPATCHED Linux kernel. This would make life a
>>> lot easier for many of our users. However, it's not going to achieve
>>> great performance. Patchless doesn't support polling, for example. And
>>> it's not clear that we can even support total-patchless Click on more
>>> recent versions of Linux, which have removed some of the hooks on
>>> which we relied.
>>>
>>> What we HAVE NOT done is separate out the 1000 lines of "real"
>>> changes. If you wanted to help with that process it would be lovely.
>>> Joonwoo, Cliff, and others have discussed some changes on the list to
>>> support newer kernels; I owe responses to them too.
>>>
>>> Eddie
>>>
>>>
>>> On 1/4/11 10:58 PM, Philip Prindeville wrote:
>>>> Ok, I just went back and looked at "click/cxxprotect.h" which
>>>> includes redefinition for C++ keywords like "new" and "virtual".
>>>>
>>>> So has anyone tried to compile Click with minimal patching (i.e. just
>>>> what is necessary to add the required instrumentation and hooks) with
>>>> a recent version of gcc (say 4.3.0 or later)?
>>>>
>>>>
>>>>
>>>> On 1/4/11 7:59 PM, Philip Prindeville wrote:
>>>>> Ok, so it seems to me that the solution is twofold:
>>>>>
>>>>> (1) require a reasonably recent version of gcc;
>>>>> (2) In places where the 'new' keyword is used in header files for
>>>>> inlines, etc. do:
>>>>>
>>>>> #define new __new
>>>>> #include<linux/list.h>
>>>>> #undef new
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 1/4/11 6:39 PM, Joonwoo Park wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I don't think 'extern "C"' made any difference. If you were able to
>>>>>> compile your example you should be able to compile even without
>>>>>> 'extern "C"' from your example.
>>>>>> Don't remember exact gcc version but IIRC earlier version of g++
>>>>>> parses '::' as namespace keyword always. To support old versions,
>>>>>> patching '::' was necessary.
>>>>>>
>>>>>> Joonwoo
>>>>>>
>>>>>> On Tue, Jan 4, 2011 at 5:35 PM, Philip Prindeville
>>>>>> <philipp_subx at redfish-solutions.com> wrote:
>>>>>>> On 1/4/11 12:25 PM, Joonwoo Park wrote:
>>>>>>>> 1 #include<iostream>
>>>>>>>> 2
>>>>>>>> 3 extern "C"
>>>>>>>> 4 {
>>>>>>>> 5 struct keyword {
>>>>>>>> 6 int new;
>>>>>>>> 7 };
>>>>>>>> 8 }
>>>>>>>> 9
>>>>>>>> 10 int main()
>>>>>>>> 11 {
>>>>>>>> 12 return 0;
>>>>>>>> 13 }
>>>>>>> I took that and modified it as:
>>>>>>>
>>>>>>> #include<iostream>
>>>>>>>
>>>>>>> extern "C"
>>>>>>> {
>>>>>>> #include<sys/types.h>
>>>>>>>
>>>>>>> void load_ldt(int32_t ldt)
>>>>>>> {
>>>>>>> asm volatile("lldt %0"::"m" (ldt));
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> int main()
>>>>>>> {
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> and it compiled fine.
>>>>>>>
>>>>>>> So that solves part of the problem.
>>>>>>>
>>>>>>> -Philip
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>