[dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/librte_eal/include/rte_common.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/include/rte_common.h b/lib/librte_eal/include/rte_common.h
index 1b630baf1..d5a32c66a 100644
--- a/lib/librte_eal/include/rte_common.h
+++ b/lib/librte_eal/include/rte_common.h
@@ -31,9 +31,11 @@ extern "C" {
 #define typeof __typeof__
 #endif
 
+#ifndef __cplusplus
 #ifndef asm
 #define asm __asm__
 #endif
+#endif
 
 /** C extension macro for environments lacking C11 features. */
 #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
-- 
2.30.0.vfs.0.2

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Thomas Monjalon]

24/03/2021 05:26, Tyler Retzlaff:
> +#ifndef __cplusplus
>  #ifndef asm
>  #define asm __asm__
>  #endif
> +#endif

It requires more explanations.
Which compilers do not define asm?
What happens with C++ if asm is undefined?

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

On Wed, Mar 24, 2021 at 09:30:58AM +0100, Thomas Monjalon wrote:
> 24/03/2021 05:26, Tyler Retzlaff:
> > +#ifndef __cplusplus
> >  #ifndef asm
> >  #define asm __asm__
> >  #endif
> > +#endif
> 
> It requires more explanations.
> Which compilers do not define asm?
> What happens with C++ if asm is undefined?

i guess the subject line on my commit didn't communicate this properly
sorry.

asm is a keyword in both the C and C++ standards. for C++ the keyword is
not permitted to be re-defined.

here is the relevant text from the standard, in particular item (2).

  17.6.4.3.1 Macro names [macro.names]

  1
  A translation unit that includes a standard library header shall not
  #define or #undef names declared in any standard library header.

  2
  A translation unit shall not #define or #undef names lexically identical
  to keywords, to the identifiers listed in Table 3, or to the attribute-tokens
  described in 7.6.

so when including rte_common.h into a translation unit that is being
compiled C++ the rte_common.h violates the standard by leaking the
#define asm macro.

this problem appears as soon as you try to build a C++ based dpdk
application with clang using windows C++ runtime and #include <list>.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Thomas Monjalon]

24/03/2021 17:45, Tyler Retzlaff:
> On Wed, Mar 24, 2021 at 09:30:58AM +0100, Thomas Monjalon wrote:
> > 24/03/2021 05:26, Tyler Retzlaff:
> > > +#ifndef __cplusplus
> > >  #ifndef asm
> > >  #define asm __asm__
> > >  #endif
> > > +#endif
> > 
> > It requires more explanations.
> > Which compilers do not define asm?
> > What happens with C++ if asm is undefined?
> 
> i guess the subject line on my commit didn't communicate this properly
> sorry.
> 
> asm is a keyword in both the C and C++ standards. for C++ the keyword is
> not permitted to be re-defined.
> 
> here is the relevant text from the standard, in particular item (2).
> 
>   17.6.4.3.1 Macro names [macro.names]
> 
>   1
>   A translation unit that includes a standard library header shall not
>   #define or #undef names declared in any standard library header.
> 
>   2
>   A translation unit shall not #define or #undef names lexically identical
>   to keywords, to the identifiers listed in Table 3, or to the attribute-tokens
>   described in 7.6.
> 
> so when including rte_common.h into a translation unit that is being
> compiled C++ the rte_common.h violates the standard by leaking the
> #define asm macro.
> 
> this problem appears as soon as you try to build a C++ based dpdk
> application with clang using windows C++ runtime and #include <list>.

I understood this part.

My question is more about the reason for having this define.
I think it is there because some compilers don't have asm keyword,
but have __asm__. And maybe that's the case for some C++ compilers.
If I'm right, this patch is breaking compilation with some
C++ compilers.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

On Wed, Mar 24, 2021 at 06:04:08PM +0100, Thomas Monjalon wrote:
> 24/03/2021 17:45, Tyler Retzlaff:
> 
> I understood this part.
> 
> My question is more about the reason for having this define.
> I think it is there because some compilers don't have asm keyword,
> but have __asm__. And maybe that's the case for some C++ compilers.
> If I'm right, this patch is breaking compilation with some
> C++ compilers.

so to qualify. you mean maybe it is breaking compilation of c++ in a
compiler that explicitly violates c++ standard when compiling c++? that
would mean it is not a c++ compiler.

in general i don't think it is a good practice to have dpdk introduce
names into the application namespace unqualified, but the point you make
is valid it can break c++ compilation if something was using this macro
as a convenience to the compiler specific extension __asm__. there will
be further issues with varying syntaxes that __asm__-style extensions
take from compiler to compiler as well.

would you prefer that i change the preprocessor protection to include only
windows? since i'm certain that this will break for any c++ compiler on
windows the moment any stl header is included.

let me know how to adjust the patch i'll submit a new version.

thanks!

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Thomas Monjalon]

24/03/2021 18:28, Tyler Retzlaff:
> On Wed, Mar 24, 2021 at 06:04:08PM +0100, Thomas Monjalon wrote:
> > 24/03/2021 17:45, Tyler Retzlaff:
> > 
> > I understood this part.
> > 
> > My question is more about the reason for having this define.
> > I think it is there because some compilers don't have asm keyword,
> > but have __asm__. And maybe that's the case for some C++ compilers.
> > If I'm right, this patch is breaking compilation with some
> > C++ compilers.
> 
> so to qualify. you mean maybe it is breaking compilation of c++ in a
> compiler that explicitly violates c++ standard when compiling c++? that
> would mean it is not a c++ compiler.

The asm keyword is part of all C++ standards?
It seems asm is non-standard in C,
that's why we use __asm__.

> in general i don't think it is a good practice to have dpdk introduce
> names into the application namespace unqualified, but the point you make
> is valid it can break c++ compilation if something was using this macro
> as a convenience to the compiler specific extension __asm__. there will
> be further issues with varying syntaxes that __asm__-style extensions
> take from compiler to compiler as well.

Yes we need to make sure there is no specific extension involved.
Is C++ asm the same as the C __asm__?

> would you prefer that i change the preprocessor protection to include only
> windows? since i'm certain that this will break for any c++ compiler on
> windows the moment any stl header is included.

No, C++ is probably the right scope.

> let me know how to adjust the patch i'll submit a new version.

I don't know yet. I would like to understand the global picture,
and have it properly documented in this commit log.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Stephen Hemminger]

On Wed, 24 Mar 2021 18:52:40 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 24/03/2021 18:28, Tyler Retzlaff:
> > On Wed, Mar 24, 2021 at 06:04:08PM +0100, Thomas Monjalon wrote:  
> > > 24/03/2021 17:45, Tyler Retzlaff:
> > > 
> > > I understood this part.
> > > 
> > > My question is more about the reason for having this define.
> > > I think it is there because some compilers don't have asm keyword,
> > > but have __asm__. And maybe that's the case for some C++ compilers.
> > > If I'm right, this patch is breaking compilation with some
> > > C++ compilers.  
> > 
> > so to qualify. you mean maybe it is breaking compilation of c++ in a
> > compiler that explicitly violates c++ standard when compiling c++? that
> > would mean it is not a c++ compiler.  
> 
> The asm keyword is part of all C++ standards?
> It seems asm is non-standard in C,
> that's why we use __asm__.
> 
> > in general i don't think it is a good practice to have dpdk introduce
> > names into the application namespace unqualified, but the point you make
> > is valid it can break c++ compilation if something was using this macro
> > as a convenience to the compiler specific extension __asm__. there will
> > be further issues with varying syntaxes that __asm__-style extensions
> > take from compiler to compiler as well.  
> 
> Yes we need to make sure there is no specific extension involved.
> Is C++ asm the same as the C __asm__?
> 
> > would you prefer that i change the preprocessor protection to include only
> > windows? since i'm certain that this will break for any c++ compiler on
> > windows the moment any stl header is included.  
> 
> No, C++ is probably the right scope.
> 
> > let me know how to adjust the patch i'll submit a new version.  
> 
> I don't know yet. I would like to understand the global picture,
> and have it properly documented in this commit log.
> 

There should be some test for C++ application use of API.
There doesn't appear to be one in the current CI suite.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

On Wed, Mar 24, 2021 at 12:52:28PM -0700, Stephen Hemminger wrote:
> On Wed, 24 Mar 2021 18:52:40 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> There should be some test for C++ application use of API.
> There doesn't appear to be one in the current CI suite.

agreed, would probably catch a few things. probably a handful of the
example applications could just have the file extension changed to .cpp
and #include any random stl header would be a start (assuming meson just
generates g++ vs gcc rules based on extension)

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

On Wed, Mar 24, 2021 at 06:52:40PM +0100, Thomas Monjalon wrote:
> 24/03/2021 18:28, Tyler Retzlaff:
> > 
> > so to qualify. you mean maybe it is breaking compilation of c++ in a
> > compiler that explicitly violates c++ standard when compiling c++? that
> > would mean it is not a c++ compiler.
> 
> The asm keyword is part of all C++ standards?
> It seems asm is non-standard in C,
> that's why we use __asm__.

the keyword is standard the meaning of the keyword is implementation
defined for both C and C++. though the C11 standard describes a
"common implementation is via statement of the form ..." [J.5.10]

> > in general i don't think it is a good practice to have dpdk introduce
> > names into the application namespace unqualified, but the point you make
> > is valid it can break c++ compilation if something was using this macro
> > as a convenience to the compiler specific extension __asm__. there will
> > be further issues with varying syntaxes that __asm__-style extensions
> > take from compiler to compiler as well.
> 
> Yes we need to make sure there is no specific extension involved.
> Is C++ asm the same as the C __asm__?

i don't think I can answer that as it depends on the compiler. there may
be no implementation at all or different implementations for asm or __asm__.
so basically neither form is portable (for an arch). though for __asm__
probably there is de-facto standardization around what gcc does and clang
mimics.

> > would you prefer that i change the preprocessor protection to include only
> > windows? since i'm certain that this will break for any c++ compiler on
> > windows the moment any stl header is included.
> 
> No, C++ is probably the right scope.
> 
> I don't know yet. I would like to understand the global picture,
> and have it properly documented in this commit log.

yep, no problem. i suspect we are probably the only ones using c++ and
dpdk (though others can speak up if they do too) which may be why this
has gone unnoticed until now.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Dmitry Kozlyuk]

2021-03-24 14:55 (UTC-0700), Tyler Retzlaff:
> On Wed, Mar 24, 2021 at 06:52:40PM +0100, Thomas Monjalon wrote:
> > 24/03/2021 18:28, Tyler Retzlaff:  
[...]
> > > would you prefer that i change the preprocessor protection to include only
> > > windows? since i'm certain that this will break for any c++ compiler on
> > > windows the moment any stl header is included.  
> > 
> > No, C++ is probably the right scope.
> > 
> > I don't know yet. I would like to understand the global picture,
> > and have it properly documented in this commit log.  
> 
> yep, no problem. i suspect we are probably the only ones using c++ and
> dpdk (though others can speak up if they do too) which may be why this
> has gone unnoticed until now.

You're certainly not the only one:

* Seastar is a C++ userspace TCP/IP stack, ScyllaDB is based on it.
* Click modular router can use DPDK from C++ [1].
* DPDK-based product out company works on is in C++.

Can't speak for Seastar and Click, but we never hit this particular issue.
What is the minimal complete example to reproduce?

[1]:
https://github.com/kohler/click/blob/master/elements/userlevel/fromdpdkdevice.cc

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

On Thu, Mar 25, 2021 at 01:41:47AM +0300, Dmitry Kozlyuk wrote:
> 2021-03-24 14:55 (UTC-0700), Tyler Retzlaff:
> 
> Can't speak for Seastar and Click, but we never hit this particular issue.
> What is the minimal complete example to reproduce?

interesting, i did a bit more digging and it is clear why. i'm consuming
headers and crt that have been enhanced to enforce c++ standards
compliance. after dumping the preprocessed output i suspect this cannot
be reproduced with the publicly available headers it seems i'm the only
"beneficiary" heh.

so the minimal example is really just to #include <list> after including
say rte_windows.h but it won't reproduce for public users of the sdk.

if there is a great deal of concern with respect to compatibility i
guess as the single consumer who has this problem i can just #undef asm
since we don't use inline asm anyway.

thoughts?

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Thomas Monjalon]

25/03/2021 01:09, Tyler Retzlaff:
> On Thu, Mar 25, 2021 at 01:41:47AM +0300, Dmitry Kozlyuk wrote:
> > 2021-03-24 14:55 (UTC-0700), Tyler Retzlaff:
> > 
> > Can't speak for Seastar and Click, but we never hit this particular issue.
> > What is the minimal complete example to reproduce?
> 
> interesting, i did a bit more digging and it is clear why. i'm consuming
> headers and crt that have been enhanced to enforce c++ standards
> compliance. after dumping the preprocessed output i suspect this cannot
> be reproduced with the publicly available headers it seems i'm the only
> "beneficiary" heh.
> 
> so the minimal example is really just to #include <list> after including
> say rte_windows.h but it won't reproduce for public users of the sdk.
> 
> if there is a great deal of concern with respect to compatibility i
> guess as the single consumer who has this problem i can just #undef asm
> since we don't use inline asm anyway.
> 
> thoughts?

No please don't do this.
There is an issue in DPDK which must be fixed.
It seems your patch is correct, I am waiting for confirmation of others.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

On Thu, Mar 25, 2021 at 09:00:54AM +0100, Thomas Monjalon wrote:
> 25/03/2021 01:09, Tyler Retzlaff:
> > On Thu, Mar 25, 2021 at 01:41:47AM +0300, Dmitry Kozlyuk wrote:
> > > 2021-03-24 14:55 (UTC-0700), Tyler Retzlaff:
> > > 
> > > Can't speak for Seastar and Click, but we never hit this particular issue.
> > > What is the minimal complete example to reproduce?
> > 
> > interesting, i did a bit more digging and it is clear why. i'm consuming
> > headers and crt that have been enhanced to enforce c++ standards
> > compliance. after dumping the preprocessed output i suspect this cannot
> > be reproduced with the publicly available headers it seems i'm the only
> > "beneficiary" heh.
> > 
> > so the minimal example is really just to #include <list> after including
> > say rte_windows.h but it won't reproduce for public users of the sdk.
> > 
> > if there is a great deal of concern with respect to compatibility i
> > guess as the single consumer who has this problem i can just #undef asm
> > since we don't use inline asm anyway.
> > 
> > thoughts?
> 
> No please don't do this.
> There is an issue in DPDK which must be fixed.
> It seems your patch is correct, I am waiting for confirmation of others.
> 

thanks, i'm glad for the desire to make the proper fix. we'll wait for
others to comment.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Thomas Monjalon]

25/03/2021 19:50, Tyler Retzlaff:
> On Thu, Mar 25, 2021 at 09:00:54AM +0100, Thomas Monjalon wrote:
> > 25/03/2021 01:09, Tyler Retzlaff:
> > > On Thu, Mar 25, 2021 at 01:41:47AM +0300, Dmitry Kozlyuk wrote:
> > > > 2021-03-24 14:55 (UTC-0700), Tyler Retzlaff:
> > > > 
> > > > Can't speak for Seastar and Click, but we never hit this particular issue.
> > > > What is the minimal complete example to reproduce?
> > > 
> > > interesting, i did a bit more digging and it is clear why. i'm consuming
> > > headers and crt that have been enhanced to enforce c++ standards
> > > compliance. after dumping the preprocessed output i suspect this cannot
> > > be reproduced with the publicly available headers it seems i'm the only
> > > "beneficiary" heh.
> > > 
> > > so the minimal example is really just to #include <list> after including
> > > say rte_windows.h but it won't reproduce for public users of the sdk.
> > > 
> > > if there is a great deal of concern with respect to compatibility i
> > > guess as the single consumer who has this problem i can just #undef asm
> > > since we don't use inline asm anyway.
> > > 
> > > thoughts?
> > 
> > No please don't do this.
> > There is an issue in DPDK which must be fixed.
> > It seems your patch is correct, I am waiting for confirmation of others.
> 
> thanks, i'm glad for the desire to make the proper fix. we'll wait for
> others to comment.

Nobody dares to comment on the asm keyword, so I'll just take your patch
with few more explanations.

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Thomas Monjalon]

24/03/2021 05:26, Tyler Retzlaff:
> --- a/lib/librte_eal/include/rte_common.h
> +++ b/lib/librte_eal/include/rte_common.h
> @@ -31,9 +31,11 @@ extern "C" {
>  #define typeof __typeof__
>  #endif
>  
> +#ifndef __cplusplus
>  #ifndef asm
>  #define asm __asm__
>  #endif
> +#endif

Applied with this explanation:

    eal: do not redefine asm keyword in C++
    
    C++ forbids redefining a keyword as a macro.
    The keyword asm is conditionally-supported and implementation defined,
    but it seems our best guess.
    
    In C, if asm does not exist, it is defined as __asm__
    which is a GNU extension.


One more question:
Can we have a similar issue with typeof?
I guess it works because typeof is not a keyword?

Re: [dpdk-dev] [PATCH] eal: standard c++ forbids defining the keyword asm as a macro

[Tyler Retzlaff]

On Tue, Apr 13, 2021 at 03:35:53PM +0200, Thomas Monjalon wrote:
> 24/03/2021 05:26, Tyler Retzlaff:
> > --- a/lib/librte_eal/include/rte_common.h
> > +++ b/lib/librte_eal/include/rte_common.h
> > @@ -31,9 +31,11 @@ extern "C" {
> >  #define typeof __typeof__
> >  #endif
> >  
> > +#ifndef __cplusplus
> >  #ifndef asm
> >  #define asm __asm__
> >  #endif
> > +#endif
> 
> Applied with this explanation:
> 
>     eal: do not redefine asm keyword in C++
>     
>     C++ forbids redefining a keyword as a macro.
>     The keyword asm is conditionally-supported and implementation defined,
>     but it seems our best guess.
>     
>     In C, if asm does not exist, it is defined as __asm__
>     which is a GNU extension.
> 
> 
> One more question:

> Can we have a similar issue with typeof?
> I guess it works because typeof is not a keyword?

typeof isn't a keyword though don't fully understand the benefit of
#define typeof __typeof__ since it just pollutes the application
namespace. extensions and intrinsics generally should stay in the
namespace reserved for use by the compiler `__'.

i would say we will have a similar issue with any keyword defined in the
c++ standard if we are defining any of them.