[Qemu-devel] [PATCH 26/67] cpu: replace command line flags with preprocessor

Michael S. Tsirkin posted 67 patches 7 years, 5 months ago
[Qemu-devel] [PATCH 26/67] cpu: replace command line flags with preprocessor
Posted by Michael S. Tsirkin 7 years, 5 months ago
Each target is currently built with a different set of include
directories, this is what makes it possible to pull in a separate copy
of cpu.h depending on the target.

Replace with per-target ifdefs which are easier to understand.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/cpu.h | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 include/cpu.h

diff --git a/include/cpu.h b/include/cpu.h
new file mode 100644
index 0000000..b18f163
--- /dev/null
+++ b/include/cpu.h
@@ -0,0 +1,2 @@
+#include "target-dir.h"
+#include TARGET_DIR(cpu.h)
-- 
MST


Re: [Qemu-devel] [PATCH 26/67] cpu: replace command line flags with preprocessor
Posted by Eric Blake 7 years, 5 months ago
On 05/03/2018 02:51 PM, Michael S. Tsirkin wrote:
> Each target is currently built with a different set of include
> directories, this is what makes it possible to pull in a separate copy
> of cpu.h depending on the target.
> 
> Replace with per-target ifdefs which are easier to understand.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   include/cpu.h | 2 ++
>   1 file changed, 2 insertions(+)
>   create mode 100644 include/cpu.h
> 
> diff --git a/include/cpu.h b/include/cpu.h
> new file mode 100644
> index 0000000..b18f163
> --- /dev/null
> +++ b/include/cpu.h
> @@ -0,0 +1,2 @@
> +#include "target-dir.h"
> +#include TARGET_DIR(cpu.h)

Out-of-order patch?  I don't see target-dir.h created anywhere earlier 
in this series.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 26/67] cpu: replace command line flags with preprocessor
Posted by Thomas Huth 7 years, 5 months ago
On 03.05.2018 21:51, Michael S. Tsirkin wrote:
> Each target is currently built with a different set of include
> directories, this is what makes it possible to pull in a separate copy
> of cpu.h depending on the target.
> 
> Replace with per-target ifdefs which are easier to understand.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/cpu.h | 2 ++
>  1 file changed, 2 insertions(+)
>  create mode 100644 include/cpu.h
> 
> diff --git a/include/cpu.h b/include/cpu.h
> new file mode 100644
> index 0000000..b18f163
> --- /dev/null
> +++ b/include/cpu.h
> @@ -0,0 +1,2 @@
> +#include "target-dir.h"
> +#include TARGET_DIR(cpu.h)

That's a *bad* idea. As far as I can see, this way, cpu.h is now
suddenly available to generic, non-target specific code. The headers in
target/* are "hidden" by purpose for all code that gets compiled via
common-obj-y and if you now make it available for everybody again, we'll
end up in ugly situation where common code might have been compiled with
target specific defines from target specific headers... Please don't do
that!

 Thomas

Re: [Qemu-devel] [PATCH 26/67] cpu: replace command line flags with preprocessor
Posted by Michael S. Tsirkin 7 years, 5 months ago
On Fri, May 04, 2018 at 02:35:49AM +0200, Thomas Huth wrote:
> On 03.05.2018 21:51, Michael S. Tsirkin wrote:
> > Each target is currently built with a different set of include
> > directories, this is what makes it possible to pull in a separate copy
> > of cpu.h depending on the target.
> > 
> > Replace with per-target ifdefs which are easier to understand.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/cpu.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >  create mode 100644 include/cpu.h
> > 
> > diff --git a/include/cpu.h b/include/cpu.h
> > new file mode 100644
> > index 0000000..b18f163
> > --- /dev/null
> > +++ b/include/cpu.h
> > @@ -0,0 +1,2 @@
> > +#include "target-dir.h"
> > +#include TARGET_DIR(cpu.h)
> 
> That's a *bad* idea. As far as I can see, this way, cpu.h is now
> suddenly available to generic, non-target specific code. The headers in
> target/* are "hidden" by purpose for all code that gets compiled via
> common-obj-y and if you now make it available for everybody again, we'll
> end up in ugly situation where common code might have been compiled with
> target specific defines from target specific headers... Please don't do
> that!
> 
>  Thomas

Are you sure that is the case? TARGET_XXX defines really shouldn't be
set for non-target specific code.

-- 
MST

Re: [Qemu-devel] [PATCH 26/67] cpu: replace command line flags with preprocessor
Posted by Thomas Huth 7 years, 5 months ago
On 04.05.2018 03:01, Michael S. Tsirkin wrote:
> On Fri, May 04, 2018 at 02:35:49AM +0200, Thomas Huth wrote:
>> On 03.05.2018 21:51, Michael S. Tsirkin wrote:
>>> Each target is currently built with a different set of include
>>> directories, this is what makes it possible to pull in a separate copy
>>> of cpu.h depending on the target.
>>>
>>> Replace with per-target ifdefs which are easier to understand.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  include/cpu.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>  create mode 100644 include/cpu.h
>>>
>>> diff --git a/include/cpu.h b/include/cpu.h
>>> new file mode 100644
>>> index 0000000..b18f163
>>> --- /dev/null
>>> +++ b/include/cpu.h
>>> @@ -0,0 +1,2 @@
>>> +#include "target-dir.h"
>>> +#include TARGET_DIR(cpu.h)
>>
>> That's a *bad* idea. As far as I can see, this way, cpu.h is now
>> suddenly available to generic, non-target specific code. The headers in
>> target/* are "hidden" by purpose for all code that gets compiled via
>> common-obj-y and if you now make it available for everybody again, we'll
>> end up in ugly situation where common code might have been compiled with
>> target specific defines from target specific headers... Please don't do
>> that!
>>
>>  Thomas
> 
> Are you sure that is the case? TARGET_XXX defines really shouldn't be
> set for non-target specific code.

Hm, ok, now I see that you've added a

#else
#error "Target-specific directory include missing"

in that target-dir.h header. So instead of not being able to include the
header since it is not in the header search path, we now will get an
error message instead... ok, that should be fine, too, so never mind.

 Thomas