[Click] Minimizing patch size

[Eddie Kohler]

The automatic header transformation is already done.

What is not done is the preparation of a minimal patch.

Eddie


On 1/9/11 6:46 PM, Philip Prindeville wrote:
> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>