Re: [pacman-dev] Clang warnings

[Allan McRae]

On 02/01/16 02:46, Olivier Brunel wrote:
> On Fri, 1 Jan 2016 15:19:19 +0100
> Rikard Falkeborn  wrote:
> 
>> Den 1 jan 2016 15:06 skrev "Olivier Brunel" :
>>>
>>> On Fri, 1 Jan 2016 23:03:22 +1000
>>> Allan McRae  wrote:
>>>  
 On 01/01/16 22:01, Olivier Brunel wrote:  
> On Thu, 31 Dec 2015 14:32:45 +0100
> Rikard Falkeborn  wrote:
>  
>> There are some warnings recently introduced when compiling with
>> Clang (with --enable-warningflags) for x86_64. The warnings are
>> related to casting of specific alpm_event-types to alpm_event_t
>> where the required alignment differs between the types. On
>> x86, it shouldn't be a problem, but maybe there are other
>> architectures pacman should work on where this is a real issue?
>> Either way, I think it would be good if pacman built cleanly
>> with Clang. Any suggestions on how to fix this?  
>
> I think the only way is not to use a specific event type but the
> generic one, to ensure the size is correct. I don't have clang
> so I didn't test this, but I think there are two ways to solve
> this.
>
> Basically the issue is when using code such as:
>
> alpm_event_hook_run_t event;
> event.type = ALPM_EVENT_HOOK_RUN_START;
> event.name = name;
> event.desc = desc;
> EVENT(handle, );
>
> So we could either use the alpm_event_t and always go into the
> right union member, e.g:
>
> alpm_event_t event;
> event.hook_run.type = ALPM_EVENT_HOOK_RUN_START;
> event.hook_run.name = name;
> event.hook_run.desc = desc;
> EVENT(handle, );
>  

 I'd prefer this way but...

 What is different with these events than all the others? Why do
 the old ones not give warnings? We should be consistent here. I
 have a feeling we have had this conversation before...

 Allan  
>>>
>>> I picked hook_run as an example, but it might affect older ones as
>>> well. The point is that alpm_event_t and alpm_event_hook_run_t
>>> differ in size. Now since the former is a union of all the specific
>>> events, it can hold them all, but when creating/using only a
>>> specific one, it might be of a different size.
>>> (Or maybe this is the first one to grow over the generic event?)
>>>
>>> As for having had this conversation before, I'm not sure, but there
>>> probaly have been a similar one in the other way around: how to use
>>> the generic event to get specific values, e.g. in pacman callback
>>> handler: typecast, of through the union.
>>>
>>> For the record, though sometimes we just go through the union (e.g.
>>> event->scriptlet_info.line) most times (or, whenever more than one
>>> or two values are needed, or used more than once) we just use a
>>> specific pointer, e.g: alpm_event_hook_run_t *e = >hook_run;
>>>
>>> -j  
>>
>> The types clang warns about do not contain pointers, I think every
>> other type does, that's why the required alignment differs. Adding a
>> dummy pointer to them makes the warnings go away.
>>
>> The warning has nothing to do with the actual size of the types but
>> where in memory they are stored (is the adress guaranteed to be an
>> even multiple of four or eight).
>>
>> Rikard
> 
> Oh, right, my bad; Apologies.
> 
> Reading the OP again, I can see that alpm_event_hook_run_t wasn't
> even at fault BTW, alpm_event_hook_t was; I just used hook_run in
> my code examples. (alpm_event_any_t was also listed, so an old one, but
> I guess it was never used that way before (we'd just use alpm_event_t),
> hence the new warning.)
> 
> My proposed solutions should still apply though, so it's only a matter
> of picking a style I guess.
> 

Use this style:

alpm_event_t event;
event.hook_run.type = ALPM_EVENT_HOOK_RUN_START;
...
EVENT(handle, )

Allan

Re: [pacman-dev] Clang warnings

[Allan McRae]

On 01/01/16 22:01, Olivier Brunel wrote:
> On Thu, 31 Dec 2015 14:32:45 +0100
> Rikard Falkeborn  wrote:
> 
>> There are some warnings recently introduced when compiling with Clang
>> (with --enable-warningflags) for x86_64. The warnings are related to
>> casting of specific alpm_event-types to alpm_event_t where the
>> required alignment differs between the types. On x86, it shouldn't be
>> a problem, but maybe there are other architectures pacman should work
>> on where this is a real issue?
>> Either way, I think it would be good if pacman built cleanly with
>> Clang. Any suggestions on how to fix this?
> 
> I think the only way is not to use a specific event type but the
> generic one, to ensure the size is correct. I don't have clang so I
> didn't test this, but I think there are two ways to solve this.
> 
> Basically the issue is when using code such as:
> 
> alpm_event_hook_run_t event;
> event.type = ALPM_EVENT_HOOK_RUN_START;
> event.name = name;
> event.desc = desc;
> EVENT(handle, );
> 
> So we could either use the alpm_event_t and always go into the right
> union member, e.g:
> 
> alpm_event_t event;
> event.hook_run.type = ALPM_EVENT_HOOK_RUN_START;
> event.hook_run.name = name;
> event.hook_run.desc = desc;
> EVENT(handle, );
> 

I'd prefer this way but...

What is different with these events than all the others? Why do the old
ones not give warnings? We should be consistent here. I have a feeling
we have had this conversation before...

Allan

Re: [pacman-dev] Clang warnings

[Olivier Brunel]

On Fri, 1 Jan 2016 23:03:22 +1000
Allan McRae  wrote:

> On 01/01/16 22:01, Olivier Brunel wrote:
> > On Thu, 31 Dec 2015 14:32:45 +0100
> > Rikard Falkeborn  wrote:
> >   
> >> There are some warnings recently introduced when compiling with
> >> Clang (with --enable-warningflags) for x86_64. The warnings are
> >> related to casting of specific alpm_event-types to alpm_event_t
> >> where the required alignment differs between the types. On x86, it
> >> shouldn't be a problem, but maybe there are other architectures
> >> pacman should work on where this is a real issue?
> >> Either way, I think it would be good if pacman built cleanly with
> >> Clang. Any suggestions on how to fix this?  
> > 
> > I think the only way is not to use a specific event type but the
> > generic one, to ensure the size is correct. I don't have clang so I
> > didn't test this, but I think there are two ways to solve this.
> > 
> > Basically the issue is when using code such as:
> > 
> > alpm_event_hook_run_t event;
> > event.type = ALPM_EVENT_HOOK_RUN_START;
> > event.name = name;
> > event.desc = desc;
> > EVENT(handle, );
> > 
> > So we could either use the alpm_event_t and always go into the right
> > union member, e.g:
> > 
> > alpm_event_t event;
> > event.hook_run.type = ALPM_EVENT_HOOK_RUN_START;
> > event.hook_run.name = name;
> > event.hook_run.desc = desc;
> > EVENT(handle, );
> >   
> 
> I'd prefer this way but...
> 
> What is different with these events than all the others? Why do the
> old ones not give warnings? We should be consistent here. I have a
> feeling we have had this conversation before...
> 
> Allan

I picked hook_run as an example, but it might affect older ones as
well. The point is that alpm_event_t and alpm_event_hook_run_t differ
in size. Now since the former is a union of all the specific events, it
can hold them all, but when creating/using only a specific one, it
might be of a different size.
(Or maybe this is the first one to grow over the generic event?)

As for having had this conversation before, I'm not sure, but there
probaly have been a similar one in the other way around: how to use the
generic event to get specific values, e.g. in pacman callback handler:
typecast, of through the union.

For the record, though sometimes we just go through the union (e.g.
event->scriptlet_info.line) most times (or, whenever more than one or
two values are needed, or used more than once) we just use a
specific pointer, e.g: alpm_event_hook_run_t *e = >hook_run;

-j

Re: [pacman-dev] Clang warnings

[Olivier Brunel]

On Thu, 31 Dec 2015 14:32:45 +0100
Rikard Falkeborn  wrote:

> There are some warnings recently introduced when compiling with Clang
> (with --enable-warningflags) for x86_64. The warnings are related to
> casting of specific alpm_event-types to alpm_event_t where the
> required alignment differs between the types. On x86, it shouldn't be
> a problem, but maybe there are other architectures pacman should work
> on where this is a real issue?
> Either way, I think it would be good if pacman built cleanly with
> Clang. Any suggestions on how to fix this?

I think the only way is not to use a specific event type but the
generic one, to ensure the size is correct. I don't have clang so I
didn't test this, but I think there are two ways to solve this.

Basically the issue is when using code such as:

alpm_event_hook_run_t event;
event.type = ALPM_EVENT_HOOK_RUN_START;
event.name = name;
event.desc = desc;
EVENT(handle, );

So we could either use the alpm_event_t and always go into the right
union member, e.g:

alpm_event_t event;
event.hook_run.type = ALPM_EVENT_HOOK_RUN_START;
event.hook_run.name = name;
event.hook_run.desc = desc;
EVENT(handle, );

Or we could use a pointer, as that may be a little less verbose, e.g:

alpm_event_t _event;
alpm_event_hook_run_t *event = &_event.hook_run;
event->type = ALPM_EVENT_HOOK_RUN_START;
event->name = name;
event->desc = desc;
EVENT(handle, &_event);

I'm not sure if using EVENT(handle, event) would work then, even though
they're basically the same in the end.

Not sure which solution you guys would prefer, if any? I'd be happy to
send a patch addressing this if you'd like though.
-j

Re: [pacman-dev] Updating bash and zsh completion

[Johannes Löthberg]

On 01/01, Allan McRae wrote:

Does anyone want to look at updating our bash adn zsh completion to
include the new options for -D and -F (and anything else that is missing)?

There is a patch for zsh here:
https://bugs.archlinux.org/index.php?do=details_id=47559

And an issue for bash completion here:
https://bugs.archlinux.org/index.php?do=details_id=47333



Should also note that there are a few other missing ones, and some flags 
that are in there are available where they shouldn't be. Like --print 
under -Q.


--
Sincerely,
 Johannes Löthberg
 PGP Key ID: 0x50FB9B273A9D0BB5
 https://theos.kyriasis.com/~kyrias/


signature.asc
Description: PGP signature

Re: [pacman-dev] Clang warnings

[Rikard Falkeborn]

Den 1 jan 2016 15:06 skrev "Olivier Brunel" :
>
> On Fri, 1 Jan 2016 23:03:22 +1000
> Allan McRae  wrote:
>
> > On 01/01/16 22:01, Olivier Brunel wrote:
> > > On Thu, 31 Dec 2015 14:32:45 +0100
> > > Rikard Falkeborn  wrote:
> > >
> > >> There are some warnings recently introduced when compiling with
> > >> Clang (with --enable-warningflags) for x86_64. The warnings are
> > >> related to casting of specific alpm_event-types to alpm_event_t
> > >> where the required alignment differs between the types. On x86, it
> > >> shouldn't be a problem, but maybe there are other architectures
> > >> pacman should work on where this is a real issue?
> > >> Either way, I think it would be good if pacman built cleanly with
> > >> Clang. Any suggestions on how to fix this?
> > >
> > > I think the only way is not to use a specific event type but the
> > > generic one, to ensure the size is correct. I don't have clang so I
> > > didn't test this, but I think there are two ways to solve this.
> > >
> > > Basically the issue is when using code such as:
> > >
> > > alpm_event_hook_run_t event;
> > > event.type = ALPM_EVENT_HOOK_RUN_START;
> > > event.name = name;
> > > event.desc = desc;
> > > EVENT(handle, );
> > >
> > > So we could either use the alpm_event_t and always go into the right
> > > union member, e.g:
> > >
> > > alpm_event_t event;
> > > event.hook_run.type = ALPM_EVENT_HOOK_RUN_START;
> > > event.hook_run.name = name;
> > > event.hook_run.desc = desc;
> > > EVENT(handle, );
> > >
> >
> > I'd prefer this way but...
> >
> > What is different with these events than all the others? Why do the
> > old ones not give warnings? We should be consistent here. I have a
> > feeling we have had this conversation before...
> >
> > Allan
>
> I picked hook_run as an example, but it might affect older ones as
> well. The point is that alpm_event_t and alpm_event_hook_run_t differ
> in size. Now since the former is a union of all the specific events, it
> can hold them all, but when creating/using only a specific one, it
> might be of a different size.
> (Or maybe this is the first one to grow over the generic event?)
>
> As for having had this conversation before, I'm not sure, but there
> probaly have been a similar one in the other way around: how to use the
> generic event to get specific values, e.g. in pacman callback handler:
> typecast, of through the union.
>
> For the record, though sometimes we just go through the union (e.g.
> event->scriptlet_info.line) most times (or, whenever more than one or
> two values are needed, or used more than once) we just use a
> specific pointer, e.g: alpm_event_hook_run_t *e = >hook_run;
>
> -j

The types clang warns about do not contain pointers, I think every other
type does, that's why the required alignment differs. Adding a dummy
pointer to them makes the warnings go away.

The warning has nothing to do with the actual size of the types but where
in memory they are stored (is the adress guaranteed to be an even multiple
of four or eight).

Rikard

Re: [pacman-dev] Clang warnings

[Olivier Brunel]

On Fri, 1 Jan 2016 15:19:19 +0100
Rikard Falkeborn  wrote:

> Den 1 jan 2016 15:06 skrev "Olivier Brunel" :
> >
> > On Fri, 1 Jan 2016 23:03:22 +1000
> > Allan McRae  wrote:
> >  
> > > On 01/01/16 22:01, Olivier Brunel wrote:  
> > > > On Thu, 31 Dec 2015 14:32:45 +0100
> > > > Rikard Falkeborn  wrote:
> > > >  
> > > >> There are some warnings recently introduced when compiling with
> > > >> Clang (with --enable-warningflags) for x86_64. The warnings are
> > > >> related to casting of specific alpm_event-types to alpm_event_t
> > > >> where the required alignment differs between the types. On
> > > >> x86, it shouldn't be a problem, but maybe there are other
> > > >> architectures pacman should work on where this is a real issue?
> > > >> Either way, I think it would be good if pacman built cleanly
> > > >> with Clang. Any suggestions on how to fix this?  
> > > >
> > > > I think the only way is not to use a specific event type but the
> > > > generic one, to ensure the size is correct. I don't have clang
> > > > so I didn't test this, but I think there are two ways to solve
> > > > this.
> > > >
> > > > Basically the issue is when using code such as:
> > > >
> > > > alpm_event_hook_run_t event;
> > > > event.type = ALPM_EVENT_HOOK_RUN_START;
> > > > event.name = name;
> > > > event.desc = desc;
> > > > EVENT(handle, );
> > > >
> > > > So we could either use the alpm_event_t and always go into the
> > > > right union member, e.g:
> > > >
> > > > alpm_event_t event;
> > > > event.hook_run.type = ALPM_EVENT_HOOK_RUN_START;
> > > > event.hook_run.name = name;
> > > > event.hook_run.desc = desc;
> > > > EVENT(handle, );
> > > >  
> > >
> > > I'd prefer this way but...
> > >
> > > What is different with these events than all the others? Why do
> > > the old ones not give warnings? We should be consistent here. I
> > > have a feeling we have had this conversation before...
> > >
> > > Allan  
> >
> > I picked hook_run as an example, but it might affect older ones as
> > well. The point is that alpm_event_t and alpm_event_hook_run_t
> > differ in size. Now since the former is a union of all the specific
> > events, it can hold them all, but when creating/using only a
> > specific one, it might be of a different size.
> > (Or maybe this is the first one to grow over the generic event?)
> >
> > As for having had this conversation before, I'm not sure, but there
> > probaly have been a similar one in the other way around: how to use
> > the generic event to get specific values, e.g. in pacman callback
> > handler: typecast, of through the union.
> >
> > For the record, though sometimes we just go through the union (e.g.
> > event->scriptlet_info.line) most times (or, whenever more than one
> > or two values are needed, or used more than once) we just use a
> > specific pointer, e.g: alpm_event_hook_run_t *e = >hook_run;
> >
> > -j  
> 
> The types clang warns about do not contain pointers, I think every
> other type does, that's why the required alignment differs. Adding a
> dummy pointer to them makes the warnings go away.
> 
> The warning has nothing to do with the actual size of the types but
> where in memory they are stored (is the adress guaranteed to be an
> even multiple of four or eight).
> 
> Rikard

Oh, right, my bad; Apologies.

Reading the OP again, I can see that alpm_event_hook_run_t wasn't
even at fault BTW, alpm_event_hook_t was; I just used hook_run in
my code examples. (alpm_event_any_t was also listed, so an old one, but
I guess it was never used that way before (we'd just use alpm_event_t),
hence the new warning.)

My proposed solutions should still apply though, so it's only a matter
of picking a style I guess.

-j