Message-ID: <20080630192954.GB6584@elte.hu>
Date:	Mon, 30 Jun 2008 21:29:54 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Glauber Costa <gcosta@...hat.com>
Cc:	linux-kernel@...r.kernel.org, tglx@...utronix.de, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 25/39] merge common parts of uaccess.


* Glauber Costa <gcosta@...hat.com> wrote:

> Ingo Molnar wrote:
>> * Glauber Costa <gcosta@...hat.com> wrote:
>>
>>> Ingo Molnar wrote:
>>>> * Glauber Costa <gcosta@...hat.com> wrote:
>>>>
>>>>> common parts of uaccess_32.h and uaccess_64.h
>>>>> are put in uaccess.h.
>>>> -tip testing found that it causes this build failure:
>>>>
>>>>   fs/binfmt_aout.c: Assembler messages:
>>>>   fs/binfmt_aout.c:152: Error: suffix or operands invalid for `cmp'
>>>>
>>>> with:
>>>>
>>>>   http://redhat.com/~mingo/misc/config-Mon_Jun_30_08_17_42_CEST_2008.bad
>>>>
>>>> and comparing the 32-bit and unified version is not simple and the  
>>>> commit is rather large.
>>>>
>>>> I'm sure the fix is simple, but this bug shows a structural problem 
>>>> with this unification patch. The proper way to unify files is to 
>>>> first bring both the 32-bit and the 64-bit version up to a unified 
>>>> form via  finegrained changes, so that uaccess_32.h and 
>>>> uaccess_64.h becomes  exactly the same file.
>>>>
>>>> ... _then_ only, in a final 'mechanic unification' step the two 
>>>> files  are merged into uaccess.h. (but no change is done to the 
>>>> content)
>>>>
>>>> If anything breaks during such a series it's bisectable to a  
>>>> finegrained patch on either the 32-bit or the 64-bit side. If this  
>>>> commit was shaped that way i could now report to you the exact  
>>>> bisection result - instead of this too-broad bisection result.
>>>>
>>>> So please rework this commit in that fashion (not just to fix this  
>>>> breakage but in anticipation of future commits) - uaccess.h is 
>>>> central  enough for us to be super careful about it.
>>>>
>>>> 	Ingo
>>> Fair.
>>>
>>> However, as I wrote in the first patch of the series, I'm not doing a 
>>>  complete unification of uaccess.h. Part of it is left for future 
>>> work,  since it's a little bit trickier.
>>>
>>> So I didn't have the option of a mechanical move. I did tried, 
>>> however,  to make sure this patch was only a code move, with 
>>> everything that is  going to the common file being equal in both 
>>> files.
>>>
>>> Needless to say, I failed. ;-) This was for a very tiny piece, but still...
>>>
>>> The options I see are:
>>>
>>> * to redo the uaccess.h unification this way, making sure a diff  
>>> between the diffs of the arch-files report nothing different, or: * 
>>> to remove the topmost patches that touches uaccess*.h, and leave only 
>>> the ones that integrate the .c and .S files, until I can really 
>>> integrate the whole of it.
>>>
>>> For the second, however, although I was careful to make incremental  
>>> changes, some small differences may exist. Examples of these  
>>> differences are places in which I introduce a few ifdefs. It's close  
>>> to nothing, but still not mechanical. Because of that, you might want 
>>> me to redo the whole series.
>>>
>>> Your call.
>>
>> well the primary worry is the build failure with gcc 4.3.1 that i've  
>> posted. If that's simple to fix we could re-try with your existing  
>> series.
>>
>> But to be defensive it's alway useful to move one component at a time.  
>> Even if you dont end up doing a mechanical unification - the stuff you  
>> move you should be able to claim to be exactly identical. I.e. the 
>> final step can be mechanic in that it unifies exactly the same content 
>> (even though both files still have remaining bits).
>>
>> Then we'll end up with nice bisection reports to the specific area that 
>> is impacted by a problem.
>>
>> 	Ingo
>
> I already have a fix for that. But I'll repost it in a way in which I 
> can claim the (part of the) files to be identical. For now, can you 
> trim the tree at that point? I think it's the best option.

sounds good. Could you git branch for that, and post the pull request 
plus the shortlog+diffstat to lkml?

To construct that branch you can merge any subset of the existing 
tip/x86/unify-lib series into that. I.e. you can cut the tree yourself 
at the point you find most appropriate - and we can then reset 
tip/x86/unify-lib and pull your tree into it.

Due to the test failure the topic is not integrated yet externally so it 
has no append-only constraints.

> As for bisection, note that I did everything with bisection in mind, 
> so I do know the importance of it. It's more a failure than a 
> fundamental mistake.

yeah, i know that and i'm not complaining :)

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists