[PATCH] build: Rename as-insn-check to as-insn-add

[PATCH] build: Rename as-insn-check to as-insn-add

[Andrew Cooper]

as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
with cc-option-add.  Update all callers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 Config.mk             | 10 +++++-----
 xen/arch/x86/Rules.mk | 14 +++++++-------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/Config.mk b/Config.mk
index c6f0df9..41683c7 100644
--- a/Config.mk
+++ b/Config.mk
@@ -163,11 +163,11 @@ as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
                        | $(filter-out -M% %.d -include %/include/xen/config.h,$(1)) \
                               -c -x c -o /dev/null - 2>&1),$(4),$(3))
 
-# as-insn-check: Add an option to compilation flags, but only if insn is
-#                supported by assembler.
-# Usage: $(call as-insn-check,CFLAGS,CC,"nop",-DHAVE_GAS_NOP)
-as-insn-check = $(eval $(call as-insn-check-closure,$(1),$(2),$(3),$(4)))
-define as-insn-check-closure
+# as-insn-add: Add an option to compilation flags, but only if insn is
+#              supported by assembler.
+# Usage: $(call as-insn-add,CFLAGS,CC,"insn",option-yes)
+as-insn-add = $(eval $(call as-insn-add-closure,$(1),$(2),$(3),$(4)))
+define as-insn-add-closure
     ifeq ($$(call as-insn,$$($(2)) $$($(1)),$(3),y,n),y)
         $(1) += $(4)
     endif
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 1dc5c37..4775336 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -14,13 +14,13 @@ CFLAGS += -msoft-float
 
 $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 $(call cc-option-add,CFLAGS,CC,-Wnested-externs)
-$(call as-insn-check,CFLAGS,CC,"vmcall",-DHAVE_GAS_VMX)
-$(call as-insn-check,CFLAGS,CC,"crc32 %eax$$(comma)%eax",-DHAVE_GAS_SSE4_2)
-$(call as-insn-check,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_GAS_EPT)
-$(call as-insn-check,CFLAGS,CC,"rdrand %eax",-DHAVE_GAS_RDRAND)
-$(call as-insn-check,CFLAGS,CC,"rdfsbase %rax",-DHAVE_GAS_FSGSBASE)
-$(call as-insn-check,CFLAGS,CC,"rdseed %eax",-DHAVE_GAS_RDSEED)
-$(call as-insn-check,CFLAGS,CC,".equ \"x\"$$(comma)1", \
+$(call as-insn-add,CFLAGS,CC,"vmcall",-DHAVE_GAS_VMX)
+$(call as-insn-add,CFLAGS,CC,"crc32 %eax$$(comma)%eax",-DHAVE_GAS_SSE4_2)
+$(call as-insn-add,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_GAS_EPT)
+$(call as-insn-add,CFLAGS,CC,"rdrand %eax",-DHAVE_GAS_RDRAND)
+$(call as-insn-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_GAS_FSGSBASE)
+$(call as-insn-add,CFLAGS,CC,"rdseed %eax",-DHAVE_GAS_RDSEED)
+$(call as-insn-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
                      -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
                      '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Jan Beulich]

>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
> with cc-option-add.  Update all callers.

I'm not convinced - cc-option-add makes relatively clear that
something is being added to the options passed to CC. If I
take as-insn-add this way, the macro would need to add an
insn to the AS invocation. While I agree as-insn-check doesn't
make clear that it adds any options, I still find this less
misleading than the suggested new name. Let's see what
others think.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Andrew Cooper]

On 22/02/18 11:33, Jan Beulich wrote:
>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
>> with cc-option-add.  Update all callers.
> I'm not convinced - cc-option-add makes relatively clear that
> something is being added to the options passed to CC. If I
> take as-insn-add this way, the macro would need to add an
> insn to the AS invocation. While I agree as-insn-check doesn't
> make clear that it adds any options, I still find this less
> misleading than the suggested new name. Let's see what
> others think.

I'm open to better name suggestions.  cc-option-add and as-insn-check
are basically the same; they make a test based on a proposed construct,
and end up mutating FLAGS.

The reason I noticed is because Rogers patch adds an option-no case to
as-insn-check.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Jan Beulich]

>>> On 22.02.18 at 12:41, <andrew.cooper3@citrix.com> wrote:
> On 22/02/18 11:33, Jan Beulich wrote:
>>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
>>> with cc-option-add.  Update all callers.
>> I'm not convinced - cc-option-add makes relatively clear that
>> something is being added to the options passed to CC. If I
>> take as-insn-add this way, the macro would need to add an
>> insn to the AS invocation. While I agree as-insn-check doesn't
>> make clear that it adds any options, I still find this less
>> misleading than the suggested new name. Let's see what
>> others think.
> 
> I'm open to better name suggestions.

The best I can come up with is, well, as-insn-check, as that
reasonably describes at least part of what the construct does.
as-insn-check-and-add-option, besides being too long, isn't
meaningfully better.

>  cc-option-add and as-insn-check
> are basically the same; they make a test based on a proposed construct,
> and end up mutating FLAGS.
> 
> The reason I noticed is because Rogers patch adds an option-no case to
> as-insn-check.

Which doesn't make the name any worse.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[George Dunlap]

On 02/22/2018 12:22 PM, Jan Beulich wrote:
>>>> On 22.02.18 at 12:41, <andrew.cooper3@citrix.com> wrote:
>> On 22/02/18 11:33, Jan Beulich wrote:
>>>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
>>>> with cc-option-add.  Update all callers.
>>> I'm not convinced - cc-option-add makes relatively clear that
>>> something is being added to the options passed to CC. If I
>>> take as-insn-add this way, the macro would need to add an
>>> insn to the AS invocation. While I agree as-insn-check doesn't
>>> make clear that it adds any options, I still find this less
>>> misleading than the suggested new name. Let's see what
>>> others think.
>>
>> I'm open to better name suggestions.
> 
> The best I can come up with is, well, as-insn-check, as that
> reasonably describes at least part of what the construct does.
> as-insn-check-and-add-option, besides being too long, isn't
> meaningfully better.

We're definitely getting into bikeshed territory here.  I agree with
Andy that 'check' doesn't really convey that something changed.  Is the
check-and-add "add it if it doesn't exist already"?  Or add it if some
other check passes / fails?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Jan Beulich]

>>> On 22.02.18 at 13:39, <george.dunlap@citrix.com> wrote:
> On 02/22/2018 12:22 PM, Jan Beulich wrote:
>>>>> On 22.02.18 at 12:41, <andrew.cooper3@citrix.com> wrote:
>>> On 22/02/18 11:33, Jan Beulich wrote:
>>>>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
>>>>> with cc-option-add.  Update all callers.
>>>> I'm not convinced - cc-option-add makes relatively clear that
>>>> something is being added to the options passed to CC. If I
>>>> take as-insn-add this way, the macro would need to add an
>>>> insn to the AS invocation. While I agree as-insn-check doesn't
>>>> make clear that it adds any options, I still find this less
>>>> misleading than the suggested new name. Let's see what
>>>> others think.
>>>
>>> I'm open to better name suggestions.
>> 
>> The best I can come up with is, well, as-insn-check, as that
>> reasonably describes at least part of what the construct does.
>> as-insn-check-and-add-option, besides being too long, isn't
>> meaningfully better.
> 
> We're definitely getting into bikeshed territory here.

Indeed, but I think a change in name should be an improvement,
not going from one questionable name to another questionable
one.

>  I agree with
> Andy that 'check' doesn't really convey that something changed.  Is the
> check-and-add "add it if it doesn't exist already"?  Or add it if some
> other check passes / fails?

It is "check if this piece of assembly assembles and add the
provided option to the indicated variable", extended by Roger's
patch to "..., and add the other provided option if it doesn't
assemble".

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Andrew Cooper]

On 22/02/18 13:39, Jan Beulich wrote:
>>>> On 22.02.18 at 13:39, <george.dunlap@citrix.com> wrote:
>> On 02/22/2018 12:22 PM, Jan Beulich wrote:
>>>>>> On 22.02.18 at 12:41, <andrew.cooper3@citrix.com> wrote:
>>>> On 22/02/18 11:33, Jan Beulich wrote:
>>>>>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
>>>>>> with cc-option-add.  Update all callers.
>>>>> I'm not convinced - cc-option-add makes relatively clear that
>>>>> something is being added to the options passed to CC. If I
>>>>> take as-insn-add this way, the macro would need to add an
>>>>> insn to the AS invocation. While I agree as-insn-check doesn't
>>>>> make clear that it adds any options, I still find this less
>>>>> misleading than the suggested new name. Let's see what
>>>>> others think.
>>>> I'm open to better name suggestions.
>>> The best I can come up with is, well, as-insn-check, as that
>>> reasonably describes at least part of what the construct does.
>>> as-insn-check-and-add-option, besides being too long, isn't
>>> meaningfully better.
>> We're definitely getting into bikeshed territory here.
> Indeed, but I think a change in name should be an improvement,
> not going from one questionable name to another questionable
> one.
>
>>  I agree with
>> Andy that 'check' doesn't really convey that something changed.  Is the
>> check-and-add "add it if it doesn't exist already"?  Or add it if some
>> other check passes / fails?
> It is "check if this piece of assembly assembles and add the
> provided option to the indicated variable", extended by Roger's
> patch to "..., and add the other provided option if it doesn't
> assemble".

Ok - how do we unblock this?

There appears to be agreement that as-insn-check isn't a great name, and
my proposed as-insn-add isn't much better.

The base runes of as-insn and cc-option are compatible.  They check the
fragment, and yield one of two options.  cc-option-add and as-insn-check
are built on top of the base runes, and mutate the flags passed in.

as-check-frag-update-option ?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Roger Pau Monné]

On Fri, Feb 23, 2018 at 11:40:34AM +0000, Andrew Cooper wrote:
> On 22/02/18 13:39, Jan Beulich wrote:
> >>>> On 22.02.18 at 13:39, <george.dunlap@citrix.com> wrote:
> >> On 02/22/2018 12:22 PM, Jan Beulich wrote:
> >>>>>> On 22.02.18 at 12:41, <andrew.cooper3@citrix.com> wrote:
> >>>> On 22/02/18 11:33, Jan Beulich wrote:
> >>>>>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
> >>>>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in line
> >>>>>> with cc-option-add.  Update all callers.
> >>>>> I'm not convinced - cc-option-add makes relatively clear that
> >>>>> something is being added to the options passed to CC. If I
> >>>>> take as-insn-add this way, the macro would need to add an
> >>>>> insn to the AS invocation. While I agree as-insn-check doesn't
> >>>>> make clear that it adds any options, I still find this less
> >>>>> misleading than the suggested new name. Let's see what
> >>>>> others think.
> >>>> I'm open to better name suggestions.
> >>> The best I can come up with is, well, as-insn-check, as that
> >>> reasonably describes at least part of what the construct does.
> >>> as-insn-check-and-add-option, besides being too long, isn't
> >>> meaningfully better.
> >> We're definitely getting into bikeshed territory here.
> > Indeed, but I think a change in name should be an improvement,
> > not going from one questionable name to another questionable
> > one.
> >
> >>  I agree with
> >> Andy that 'check' doesn't really convey that something changed.  Is the
> >> check-and-add "add it if it doesn't exist already"?  Or add it if some
> >> other check passes / fails?
> > It is "check if this piece of assembly assembles and add the
> > provided option to the indicated variable", extended by Roger's
> > patch to "..., and add the other provided option if it doesn't
> > assemble".
> 
> Ok - how do we unblock this?
> 
> There appears to be agreement that as-insn-check isn't a great name, and
> my proposed as-insn-add isn't much better.
> 
> The base runes of as-insn and cc-option are compatible.  They check the
> fragment, and yield one of two options.  cc-option-add and as-insn-check
> are built on top of the base runes, and mutate the flags passed in.
> 
> as-check-frag-update-option ?

That seems overly long, and TBH I think almost everyone not familiar
with the code would have to go an look what this macro does.

I'm fine with either as-insn-check, as-insn-add or as-option-add, but
I would also like to unblock this. I guess if there's no consensus we
just leave the current one?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] build: Rename as-insn-check to as-insn-add"):
> Ok - how do we unblock this?
> 
> There appears to be agreement that as-insn-check isn't a great name, and
> my proposed as-insn-add isn't much better.
> 
> The base runes of as-insn and cc-option are compatible.  They check the
> fragment, and yield one of two options.  cc-option-add and as-insn-check
> are built on top of the base runes, and mutate the flags passed in.
> 
> as-check-frag-update-option ?

My preferred bikeshed colour is `as-insn-add'.  I agree that simply
`check' is wrong but would be content with almost anything else.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Jan Beulich]

>>> On 23.02.18 at 12:40, <andrew.cooper3@citrix.com> wrote:
> On 22/02/18 13:39, Jan Beulich wrote:
>>>>> On 22.02.18 at 13:39, <george.dunlap@citrix.com> wrote:
>>> On 02/22/2018 12:22 PM, Jan Beulich wrote:
>>>>>>> On 22.02.18 at 12:41, <andrew.cooper3@citrix.com> wrote:
>>>>> On 22/02/18 11:33, Jan Beulich wrote:
>>>>>>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in 
> line
>>>>>>> with cc-option-add.  Update all callers.
>>>>>> I'm not convinced - cc-option-add makes relatively clear that
>>>>>> something is being added to the options passed to CC. If I
>>>>>> take as-insn-add this way, the macro would need to add an
>>>>>> insn to the AS invocation. While I agree as-insn-check doesn't
>>>>>> make clear that it adds any options, I still find this less
>>>>>> misleading than the suggested new name. Let's see what
>>>>>> others think.
>>>>> I'm open to better name suggestions.
>>>> The best I can come up with is, well, as-insn-check, as that
>>>> reasonably describes at least part of what the construct does.
>>>> as-insn-check-and-add-option, besides being too long, isn't
>>>> meaningfully better.
>>> We're definitely getting into bikeshed territory here.
>> Indeed, but I think a change in name should be an improvement,
>> not going from one questionable name to another questionable
>> one.
>>
>>>  I agree with
>>> Andy that 'check' doesn't really convey that something changed.  Is the
>>> check-and-add "add it if it doesn't exist already"?  Or add it if some
>>> other check passes / fails?
>> It is "check if this piece of assembly assembles and add the
>> provided option to the indicated variable", extended by Roger's
>> patch to "..., and add the other provided option if it doesn't
>> assemble".
> 
> Ok - how do we unblock this?
> 
> There appears to be agreement that as-insn-check isn't a great name, and
> my proposed as-insn-add isn't much better.
> 
> The base runes of as-insn and cc-option are compatible.  They check the
> fragment, and yield one of two options.  cc-option-add and as-insn-check
> are built on top of the base runes, and mutate the flags passed in.
> 
> as-check-frag-update-option ?

as-insn-option-add? Or just as-option-add, considering Roger's
new use cases which don't check insns?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] build: Rename as-insn-check to as-insn-add

[Andrew Cooper]

On 23/02/18 11:58, Jan Beulich wrote:
>>>> On 23.02.18 at 12:40, <andrew.cooper3@citrix.com> wrote:
>> On 22/02/18 13:39, Jan Beulich wrote:
>>>>>> On 22.02.18 at 13:39, <george.dunlap@citrix.com> wrote:
>>>> On 02/22/2018 12:22 PM, Jan Beulich wrote:
>>>>>>>> On 22.02.18 at 12:41, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 22/02/18 11:33, Jan Beulich wrote:
>>>>>>>>>> On 22.02.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> as-insn-check mutates the passed-in flags.  Rename it to as-insn-add, in 
>> line
>>>>>>>> with cc-option-add.  Update all callers.
>>>>>>> I'm not convinced - cc-option-add makes relatively clear that
>>>>>>> something is being added to the options passed to CC. If I
>>>>>>> take as-insn-add this way, the macro would need to add an
>>>>>>> insn to the AS invocation. While I agree as-insn-check doesn't
>>>>>>> make clear that it adds any options, I still find this less
>>>>>>> misleading than the suggested new name. Let's see what
>>>>>>> others think.
>>>>>> I'm open to better name suggestions.
>>>>> The best I can come up with is, well, as-insn-check, as that
>>>>> reasonably describes at least part of what the construct does.
>>>>> as-insn-check-and-add-option, besides being too long, isn't
>>>>> meaningfully better.
>>>> We're definitely getting into bikeshed territory here.
>>> Indeed, but I think a change in name should be an improvement,
>>> not going from one questionable name to another questionable
>>> one.
>>>
>>>>  I agree with
>>>> Andy that 'check' doesn't really convey that something changed.  Is the
>>>> check-and-add "add it if it doesn't exist already"?  Or add it if some
>>>> other check passes / fails?
>>> It is "check if this piece of assembly assembles and add the
>>> provided option to the indicated variable", extended by Roger's
>>> patch to "..., and add the other provided option if it doesn't
>>> assemble".
>> Ok - how do we unblock this?
>>
>> There appears to be agreement that as-insn-check isn't a great name, and
>> my proposed as-insn-add isn't much better.
>>
>> The base runes of as-insn and cc-option are compatible.  They check the
>> fragment, and yield one of two options.  cc-option-add and as-insn-check
>> are built on top of the base runes, and mutate the flags passed in.
>>
>> as-check-frag-update-option ?
> as-insn-option-add? Or just as-option-add, considering Roger's
> new use cases which don't check insns?

Lets go with as-option-add.  I'm happy with that.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel