[PATCH] tools/foreign: Avoid using alignment directives when not appropriate

[PATCH] tools/foreign: Avoid using alignment directives when not appropriate

[Andrew Cooper]

The foreign header generation blindly replaces 'uint64_t' with '__align8__
uint64_t', to get correct alignment when built as 32bit.  This is correct in
most circumstances, but Clang objects to two specific uses.

 * Inside a sizeof() expression
 * As part of a typecast

An example error looks like:

/local/xen.git/tools/libxc/../../tools/include/xen/foreign/x86_64.h:204:44:
error: 'aligned' attribute ignored when parsing type [-Werror,-Wignored-attributes]
    __align8__ uint64_t evtchn_mask[sizeof(__align8__ uint64_t) * 8];
                                           ^~~~~~~~~~
/local/xen.git/tools/libxc/../../tools/include/xen/foreign/x86_64.h:13:36:
note: expanded from macro '__align8__'
# define __align8__ __attribute__((aligned (8)))
                                   ^~~~~~~~~~~

This sedary is sufficient to fix all the bad examples without touching any of
the legitimate uses, and is more simple than teaching mkheader.py how to parse
C.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/include/xen-foreign/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/include/xen-foreign/Makefile b/tools/include/xen-foreign/Makefile
index 80a446a..b25bfa8 100644
--- a/tools/include/xen-foreign/Makefile
+++ b/tools/include/xen-foreign/Makefile
@@ -35,6 +35,8 @@ x86_32.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/
 
 x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h
 	$(PYTHON) $< $* $@ $(filter %.h,$^)
+	#Avoid mixing an alignment directive with a uint64_t cast or sizeof expression
+	sed 's/(__align8__ uint64_t)/(uint64_t)/g' -i $@
 
 checker.c: mkchecker.py structs.py
 	$(PYTHON) $< $@ $(architectures)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/foreign: Avoid using alignment directives when not appropriate

[Jan Beulich]

>>> On 07.03.16 at 19:28, <andrew.cooper3@citrix.com> wrote:
> --- a/tools/include/xen-foreign/Makefile
> +++ b/tools/include/xen-foreign/Makefile
> @@ -35,6 +35,8 @@ x86_32.h: mkheader.py structs.py 
> $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/
>  
>  x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h 
> $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h
>  	$(PYTHON) $< $* $@ $(filter %.h,$^)
> +	#Avoid mixing an alignment directive with a uint64_t cast or sizeof expression
> +	sed 's/(__align8__ uint64_t)/(uint64_t)/g' -i $@

A two step rule like this should make use of a temporary file, to
avoid breakage when the build process gets interrupted between
the two steps.

And then - is it perhaps worth to generalize the pattern in one or
more of a couple of possible ways? Considering int64_t uses
would perhaps be the most relevant one (even if not needed
right away). But of course this could get as generic as

s/(__align[0-9]*__ \([a-z0-9_]*\))/(\1)/g

without - afaict (based on your commit description) - breaking
anything.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/foreign: Avoid using alignment directives when not appropriate

[Andrew Cooper]

On 08/03/16 09:54, Jan Beulich wrote:
>>>> On 07.03.16 at 19:28, <andrew.cooper3@citrix.com> wrote:
>> --- a/tools/include/xen-foreign/Makefile
>> +++ b/tools/include/xen-foreign/Makefile
>> @@ -35,6 +35,8 @@ x86_32.h: mkheader.py structs.py 
>> $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/
>>  
>>  x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h 
>> $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h
>>  	$(PYTHON) $< $* $@ $(filter %.h,$^)
>> +	#Avoid mixing an alignment directive with a uint64_t cast or sizeof expression
>> +	sed 's/(__align8__ uint64_t)/(uint64_t)/g' -i $@
> A two step rule like this should make use of a temporary file, to
> avoid breakage when the build process gets interrupted between
> the two steps.
>
> And then - is it perhaps worth to generalize the pattern in one or
> more of a couple of possible ways? Considering int64_t uses
> would perhaps be the most relevant one (even if not needed
> right away). But of course this could get as generic as
>
> s/(__align[0-9]*__ \([a-z0-9_]*\))/(\1)/g
>
> without - afaict (based on your commit description) - breaking
> anything.

Both of these would want to be + rather than * to ensure some content.

While generic is usually better, in this case I think it is better to
stick with the most specific fix, to reduce the risk of accidentally
clobbering a real __align__.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/foreign: Avoid using alignment directives when not appropriate

[Jan Beulich]

>>> On 08.03.16 at 12:19, <andrew.cooper3@citrix.com> wrote:
> On 08/03/16 09:54, Jan Beulich wrote:
>>>>> On 07.03.16 at 19:28, <andrew.cooper3@citrix.com> wrote:
>>> --- a/tools/include/xen-foreign/Makefile
>>> +++ b/tools/include/xen-foreign/Makefile
>>> @@ -35,6 +35,8 @@ x86_32.h: mkheader.py structs.py 
>>> $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/
>>>  
>>>  x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h 
>>> $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h
>>>  	$(PYTHON) $< $* $@ $(filter %.h,$^)
>>> +	#Avoid mixing an alignment directive with a uint64_t cast or sizeof 
> expression
>>> +	sed 's/(__align8__ uint64_t)/(uint64_t)/g' -i $@
>> A two step rule like this should make use of a temporary file, to
>> avoid breakage when the build process gets interrupted between
>> the two steps.
>>
>> And then - is it perhaps worth to generalize the pattern in one or
>> more of a couple of possible ways? Considering int64_t uses
>> would perhaps be the most relevant one (even if not needed
>> right away). But of course this could get as generic as
>>
>> s/(__align[0-9]*__ \([a-z0-9_]*\))/(\1)/g
>>
>> without - afaict (based on your commit description) - breaking
>> anything.
> 
> Both of these would want to be + rather than * to ensure some content.

True. I had just avoided them because they would also have needed
escaping.

> While generic is usually better, in this case I think it is better to
> stick with the most specific fix, to reduce the risk of accidentally
> clobbering a real __align__.

Well, as your commit description alludes to, there are no
syntactically correct uses of just an attribute and a type in the
context of other than sizeof(), typeof(), or a cast. Hence I
wouldn't view the generalization as potentially problematic, but
otoh I can understand you trying to be conservative. Hence the
minimal suggestion of at least also dealing with int64_t. But in
the end it's the tools maintainers' call anyway.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel