This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Speed-up sprintf() family of functions.


Jonathan Larmour <jifl@eCosCentric.com> writes:

> Sergei Organov wrote:
>> Jonathan Larmour <jifl@eCosCentric.com> writes:
>>
>>
>>>Sergei Organov wrote:
>>>
>>>>Andrew Lunn <andrew@lunn.ch> writes:
>>>>
>>>>
>>>>>>The updated patch is attached.
>>>>>>
>>>>>
>>>>>Hi Sergei
>>>>>
>>>>>Im looking at the patch now ready for including into anoncvs. I have
>>>>>one minor problem which needs fixing. When i compile for the linux
>>>>>target with gcc 4.1.2 i get a warning:
>>>>
>>>>
>>>>There doesn't seem to be a way to actually fix this warning, only some
>>>>hacks to shut-up GCC. What hack in particular do you use to prevent this
>>>>kind of warnings in other places of eCos where it occurs?
>>>
>>>There is a correct solution. Create a one-off union type, containing a
>>>FILE and a Cyg_VsnprintfStream and assign the stream to the
>>>Cyg_VsnprintfStream, and then take the address of the FILE
>>>member. Alignment constraints are dealt with by the fact that streams
>>>are created with the correct alignment - but GCC doesn't know that.
>>
>>
>> Well, I can definitely do that, but I don't think it's "correct"
>> solution, -- it's one kind of ugly hacks that will make GCC
>> shut-up.
>
> Strictly it's the correct solution. I was involved in some of the
> discussion when this was first implemented in GCC. A union is the
> official and permanently portable way to solve it. Yes it's somewhat
> pointless syntactic sugar, but ......

Oops, sorry, I've misread your union suggestion. I thought you've
proposed to put FILE* and Cyg_Vsnprintf* into the union, but you've
suggested to put FILE and Cyg_Vsnprintf themselves into the union. That
simply won't work as Cyg_Vsnprintf type, being non-POD, can't be a union
member :(

>
>> Alignment has nothing to do about it, I believe, -- gcc warns
>> about possible aliasing rules violation, not alignment.
>
> Indeed, but separate from aliasing specifically, casting between two
> types can cause problems if one of the types has stronger alignment
> constraints.

Yes, indeed, but putting 2 pointers into a union won't fix alignment
problems anyway. Besides, the FILE type is defined so that there is no
alignment problem here.

[...]

>> Another way to "fix" it is:
>>
>> -    return vfnprintf( (FILE *)&stream, size, format, arg );
>> +    return vfnprintf( (FILE *)(void*)&stream, size, format, arg );
>>
>> is it "correct" fix?
>
> "Correct", no.

I'd say: correct, no; "correct", yes ;) Anyway, please see below for a
solution that I think is correct.

> But I take your point about not actually dereferencing the pointer
> here, so aliasing is never, in practice, going to be an issue.

Exactly.

> So let's just go with this one, since it's simple. I'm about to
> go offline for an extended period, so it can wait for me to return, or
> perhaps Andrew can check it in with just that change.

It's OK with me to go with this one, though I think I've found a better
way to fix this. As the way I've found is unrelated to my patch, I put
it here for consideration in the future:

I'd re-define the FILE type this way:

-typedef CYG_ADDRESS FILE[9999];
+typedef CYG_ADDRESS __attribute__((__may_alias__)) FILE[9999];

As no dereferences of FILE* pointer are ever made, this should be
harmless from the point of view of code generation, but it does prevent
GCC from producing any aliasing warnings for pointers conversion to/from
FILE* even when -Wstrict-aliasing=2 is used.

-- Sergei Organov.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]