[Click] Minimizing patch size

[Eddie Kohler]

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.

> 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.
2. Given automatic header transformation why would you care?
3. If you have a specific patch or correct version number do send it for 
further discussion.

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
>>>>>>
>>>>>>
>>>>>>
>>>>
>