| Message ID | 1457375322-7975-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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
>>> 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
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)
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(+)