[PATCHv2] linux-generic/include/odp_packet_io_internal: add visability hidden

Message ID 1460150090-29253-1-git-send-email-anders.roxell@linaro.org
State New
Headers show

Commit Message

Anders Roxell April 8, 2016, 9:14 p.m.
Internal functions should not be part of symbols is visible outside the
library.

Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
 .../linux-generic/include/odp_packet_io_internal.h | 23 +++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Bill Fischofer April 9, 2016, 3:24 p.m. | #1
Why is this restricted to GCC?  What happens when compiling with clang?  A
quick experiment shows clang accepts the same __attribute__ without
complaint but not sure what it does with it.

On Fri, Apr 8, 2016 at 4:14 PM, Anders Roxell <anders.roxell@linaro.org>
wrote:

> Internal functions should not be part of symbols is visible outside the

> library.

>

> Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>

> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

> ---

>  .../linux-generic/include/odp_packet_io_internal.h | 23

> +++++++++++++---------

>  1 file changed, 14 insertions(+), 9 deletions(-)

>

> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h

> b/platform/linux-generic/include/odp_packet_io_internal.h

> index cca5c39..5c40c51 100644

> --- a/platform/linux-generic/include/odp_packet_io_internal.h

> +++ b/platform/linux-generic/include/odp_packet_io_internal.h

> @@ -45,6 +45,11 @@ extern "C" {

>   *  requested number of packets were not handled. */

>  #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e != EINTR)

>

> +#if defined(__GNUC__)

> +#  define HIDDEN  __attribute__((visibility("hidden")))

> +#else

> +#  define HIDDEN

> +#endif

>  /* Forward declaration */

>  struct pktio_if_ops;

>

> @@ -171,7 +176,7 @@ typedef struct pktio_if_ops {

>  int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,

>                         uint16_t buf_len, odp_packet_t *pkt_ret);

>

> -extern void *pktio_entry_ptr[];

> +extern void HIDDEN *pktio_entry_ptr[];

>

>  static inline int pktio_to_id(odp_pktio_t pktio)

>  {

> @@ -217,16 +222,16 @@ int single_recv_queue(pktio_entry_t *entry, int

> index, odp_packet_t packets[],

>  int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t

> packets[],

>                       int num);

>

> -extern const pktio_if_ops_t netmap_pktio_ops;

> -extern const pktio_if_ops_t dpdk_pktio_ops;

> -extern const pktio_if_ops_t sock_mmsg_pktio_ops;

> -extern const pktio_if_ops_t sock_mmap_pktio_ops;

> -extern const pktio_if_ops_t loopback_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;

>  #ifdef HAVE_PCAP

> -extern const pktio_if_ops_t pcap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;

>  #endif

> -extern const pktio_if_ops_t tap_pktio_ops;

> -extern const pktio_if_ops_t * const pktio_if_ops[];

> +extern const HIDDEN pktio_if_ops_t tap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];

>

>  int sysfs_stats(pktio_entry_t *pktio_entry,

>                 odp_pktio_stats_t *stats);

> --

> 2.8.0.rc3

>

> _______________________________________________

> lng-odp mailing list

> lng-odp@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp

>
Mike Holmes April 9, 2016, 6:07 p.m. | #2
On 8 April 2016 at 17:14, Anders Roxell <anders.roxell@linaro.org> wrote:

> Internal functions should not be part of symbols is visible outside the

> library.

>

> Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>

> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

> ---

>  .../linux-generic/include/odp_packet_io_internal.h | 23

> +++++++++++++---------

>  1 file changed, 14 insertions(+), 9 deletions(-)

>

> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h

> b/platform/linux-generic/include/odp_packet_io_internal.h

> index cca5c39..5c40c51 100644

> --- a/platform/linux-generic/include/odp_packet_io_internal.h

> +++ b/platform/linux-generic/include/odp_packet_io_internal.h

> @@ -45,6 +45,11 @@ extern "C" {

>   *  requested number of packets were not handled. */

>  #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e != EINTR)

>

> +#if defined(__GNUC__)

> +#  define HIDDEN  __attribute__((visibility("hidden")))

>


Should this be in the compiler hints.h with the others ?

include/odp/api/spec/hints.h:#define ODP_NORETURN
__attribute__((__noreturn__))
include/odp/api/spec/hints.h:#define ODP_WEAK_SYMBOL
__attribute__((__weak__))
include/odp/api/spec/hints.h:#define ODP_HOT_CODE
 __attribute__((__hot__))
include/odp/api/spec/hints.h:#define ODP_COLD_CODE
__attribute__((__cold__))

If it is part of the spec it is easier for other platforms to hide their
internals by reusing it.



> +#else

> +#  define HIDDEN

> +#endif

>  /* Forward declaration */

>  struct pktio_if_ops;

>

> @@ -171,7 +176,7 @@ typedef struct pktio_if_ops {

>  int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,

>                         uint16_t buf_len, odp_packet_t *pkt_ret);

>

> -extern void *pktio_entry_ptr[];

> +extern void HIDDEN *pktio_entry_ptr[];

>

>  static inline int pktio_to_id(odp_pktio_t pktio)

>  {

> @@ -217,16 +222,16 @@ int single_recv_queue(pktio_entry_t *entry, int

> index, odp_packet_t packets[],

>  int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t

> packets[],

>                       int num);

>

> -extern const pktio_if_ops_t netmap_pktio_ops;

> -extern const pktio_if_ops_t dpdk_pktio_ops;

> -extern const pktio_if_ops_t sock_mmsg_pktio_ops;

> -extern const pktio_if_ops_t sock_mmap_pktio_ops;

> -extern const pktio_if_ops_t loopback_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;

>  #ifdef HAVE_PCAP

> -extern const pktio_if_ops_t pcap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;

>  #endif

> -extern const pktio_if_ops_t tap_pktio_ops;

> -extern const pktio_if_ops_t * const pktio_if_ops[];

> +extern const HIDDEN pktio_if_ops_t tap_pktio_ops;

> +extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];

>

>  int sysfs_stats(pktio_entry_t *pktio_entry,

>                 odp_pktio_stats_t *stats);

> --

> 2.8.0.rc3

>

> _______________________________________________

> lng-odp mailing list

> lng-odp@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp

>




-- 
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
"Work should be fun and collaborative, the rest follows"
Anders Roxell April 11, 2016, 8:36 p.m. | #3
On 9 April 2016 at 17:24, Bill Fischofer <bill.fischofer@linaro.org> wrote:
> Why is this restricted to GCC?  What happens when compiling with clang?  A
> quick experiment shows clang accepts the same __attribute__ without
> complaint but not sure what it does with it.

Clang works without a problem, so I don't think there is anything to do.

Cheers,
Anders

>
> On Fri, Apr 8, 2016 at 4:14 PM, Anders Roxell <anders.roxell@linaro.org>
> wrote:
>>
>> Internal functions should not be part of symbols is visible outside the
>> library.
>>
>> Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>
>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>> ---
>>  .../linux-generic/include/odp_packet_io_internal.h | 23
>> +++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h
>> b/platform/linux-generic/include/odp_packet_io_internal.h
>> index cca5c39..5c40c51 100644
>> --- a/platform/linux-generic/include/odp_packet_io_internal.h
>> +++ b/platform/linux-generic/include/odp_packet_io_internal.h
>> @@ -45,6 +45,11 @@ extern "C" {
>>   *  requested number of packets were not handled. */
>>  #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e !=
>> EINTR)
>>
>> +#if defined(__GNUC__)
>> +#  define HIDDEN  __attribute__((visibility("hidden")))
>> +#else
>> +#  define HIDDEN
>> +#endif
>>  /* Forward declaration */
>>  struct pktio_if_ops;
>>
>> @@ -171,7 +176,7 @@ typedef struct pktio_if_ops {
>>  int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,
>>                         uint16_t buf_len, odp_packet_t *pkt_ret);
>>
>> -extern void *pktio_entry_ptr[];
>> +extern void HIDDEN *pktio_entry_ptr[];
>>
>>  static inline int pktio_to_id(odp_pktio_t pktio)
>>  {
>> @@ -217,16 +222,16 @@ int single_recv_queue(pktio_entry_t *entry, int
>> index, odp_packet_t packets[],
>>  int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t
>> packets[],
>>                       int num);
>>
>> -extern const pktio_if_ops_t netmap_pktio_ops;
>> -extern const pktio_if_ops_t dpdk_pktio_ops;
>> -extern const pktio_if_ops_t sock_mmsg_pktio_ops;
>> -extern const pktio_if_ops_t sock_mmap_pktio_ops;
>> -extern const pktio_if_ops_t loopback_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;
>>  #ifdef HAVE_PCAP
>> -extern const pktio_if_ops_t pcap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;
>>  #endif
>> -extern const pktio_if_ops_t tap_pktio_ops;
>> -extern const pktio_if_ops_t * const pktio_if_ops[];
>> +extern const HIDDEN pktio_if_ops_t tap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];
>>
>>  int sysfs_stats(pktio_entry_t *pktio_entry,
>>                 odp_pktio_stats_t *stats);
>> --
>> 2.8.0.rc3
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Anders Roxell April 11, 2016, 8:39 p.m. | #4
On 9 April 2016 at 20:07, Mike Holmes <mike.holmes@linaro.org> wrote:
>
>
> On 8 April 2016 at 17:14, Anders Roxell <anders.roxell@linaro.org> wrote:
>>
>> Internal functions should not be part of symbols is visible outside the
>> library.
>>
>> Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>
>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>> ---
>>  .../linux-generic/include/odp_packet_io_internal.h | 23
>> +++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h
>> b/platform/linux-generic/include/odp_packet_io_internal.h
>> index cca5c39..5c40c51 100644
>> --- a/platform/linux-generic/include/odp_packet_io_internal.h
>> +++ b/platform/linux-generic/include/odp_packet_io_internal.h
>> @@ -45,6 +45,11 @@ extern "C" {
>>   *  requested number of packets were not handled. */
>>  #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e !=
>> EINTR)
>>
>> +#if defined(__GNUC__)
>> +#  define HIDDEN  __attribute__((visibility("hidden")))
>
>
> Should this be in the compiler hints.h with the others ?

either there or in in platform/linux-generic/include/odp_internal.h
since its an internal define.

Cheers,
Anders

>
> include/odp/api/spec/hints.h:#define ODP_NORETURN
> __attribute__((__noreturn__))
> include/odp/api/spec/hints.h:#define ODP_WEAK_SYMBOL
> __attribute__((__weak__))
> include/odp/api/spec/hints.h:#define ODP_HOT_CODE
> __attribute__((__hot__))
> include/odp/api/spec/hints.h:#define ODP_COLD_CODE
> __attribute__((__cold__))
>
> If it is part of the spec it is easier for other platforms to hide their
> internals by reusing it.
>
>
>>
>> +#else
>> +#  define HIDDEN
>> +#endif
>>  /* Forward declaration */
>>  struct pktio_if_ops;
>>
>> @@ -171,7 +176,7 @@ typedef struct pktio_if_ops {
>>  int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,
>>                         uint16_t buf_len, odp_packet_t *pkt_ret);
>>
>> -extern void *pktio_entry_ptr[];
>> +extern void HIDDEN *pktio_entry_ptr[];
>>
>>  static inline int pktio_to_id(odp_pktio_t pktio)
>>  {
>> @@ -217,16 +222,16 @@ int single_recv_queue(pktio_entry_t *entry, int
>> index, odp_packet_t packets[],
>>  int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t
>> packets[],
>>                       int num);
>>
>> -extern const pktio_if_ops_t netmap_pktio_ops;
>> -extern const pktio_if_ops_t dpdk_pktio_ops;
>> -extern const pktio_if_ops_t sock_mmsg_pktio_ops;
>> -extern const pktio_if_ops_t sock_mmap_pktio_ops;
>> -extern const pktio_if_ops_t loopback_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;
>>  #ifdef HAVE_PCAP
>> -extern const pktio_if_ops_t pcap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;
>>  #endif
>> -extern const pktio_if_ops_t tap_pktio_ops;
>> -extern const pktio_if_ops_t * const pktio_if_ops[];
>> +extern const HIDDEN pktio_if_ops_t tap_pktio_ops;
>> +extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];
>>
>>  int sysfs_stats(pktio_entry_t *pktio_entry,
>>                 odp_pktio_stats_t *stats);
>> --
>> 2.8.0.rc3
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
> --
> Mike Holmes
> Technical Manager - Linaro Networking Group
> Linaro.org │ Open source software for ARM SoCs
> "Work should be fun and collaborative, the rest follows"
>
>
Mike Holmes April 11, 2016, 8:45 p.m. | #5
On 11 April 2016 at 16:39, Anders Roxell <anders.roxell@linaro.org> wrote:

> On 9 April 2016 at 20:07, Mike Holmes <mike.holmes@linaro.org> wrote:

> >

> >

> > On 8 April 2016 at 17:14, Anders Roxell <anders.roxell@linaro.org>

> wrote:

> >>

> >> Internal functions should not be part of symbols is visible outside the

> >> library.

> >>

> >> Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>

> >> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

> >> ---

> >>  .../linux-generic/include/odp_packet_io_internal.h | 23

> >> +++++++++++++---------

> >>  1 file changed, 14 insertions(+), 9 deletions(-)

> >>

> >> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h

> >> b/platform/linux-generic/include/odp_packet_io_internal.h

> >> index cca5c39..5c40c51 100644

> >> --- a/platform/linux-generic/include/odp_packet_io_internal.h

> >> +++ b/platform/linux-generic/include/odp_packet_io_internal.h

> >> @@ -45,6 +45,11 @@ extern "C" {

> >>   *  requested number of packets were not handled. */

> >>  #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e !=

> >> EINTR)

> >>

> >> +#if defined(__GNUC__)

> >> +#  define HIDDEN  __attribute__((visibility("hidden")))

> >

> >

> > Should this be in the compiler hints.h with the others ?

>

> either there or in in platform/linux-generic/include/odp_internal.h

> since its an internal define.

>


I am ok with it being internal too, but then on principle I'd like to see
the other internal hints removed from the public API files.
At that point we will have reduced the public API surface area to only what
it needs to contain.


>

> Cheers,

> Anders

>

> >

> > include/odp/api/spec/hints.h:#define ODP_NORETURN

> > __attribute__((__noreturn__))

> > include/odp/api/spec/hints.h:#define ODP_WEAK_SYMBOL

> > __attribute__((__weak__))

> > include/odp/api/spec/hints.h:#define ODP_HOT_CODE

> > __attribute__((__hot__))

> > include/odp/api/spec/hints.h:#define ODP_COLD_CODE

> > __attribute__((__cold__))

> >

> > If it is part of the spec it is easier for other platforms to hide their

> > internals by reusing it.

> >

> >

> >>

> >> +#else

> >> +#  define HIDDEN

> >> +#endif

> >>  /* Forward declaration */

> >>  struct pktio_if_ops;

> >>

> >> @@ -171,7 +176,7 @@ typedef struct pktio_if_ops {

> >>  int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t

> *base,

> >>                         uint16_t buf_len, odp_packet_t *pkt_ret);

> >>

> >> -extern void *pktio_entry_ptr[];

> >> +extern void HIDDEN *pktio_entry_ptr[];

> >>

> >>  static inline int pktio_to_id(odp_pktio_t pktio)

> >>  {

> >> @@ -217,16 +222,16 @@ int single_recv_queue(pktio_entry_t *entry, int

> >> index, odp_packet_t packets[],

> >>  int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t

> >> packets[],

> >>                       int num);

> >>

> >> -extern const pktio_if_ops_t netmap_pktio_ops;

> >> -extern const pktio_if_ops_t dpdk_pktio_ops;

> >> -extern const pktio_if_ops_t sock_mmsg_pktio_ops;

> >> -extern const pktio_if_ops_t sock_mmap_pktio_ops;

> >> -extern const pktio_if_ops_t loopback_pktio_ops;

> >> +extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;

> >> +extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;

> >> +extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;

> >> +extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;

> >> +extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;

> >>  #ifdef HAVE_PCAP

> >> -extern const pktio_if_ops_t pcap_pktio_ops;

> >> +extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;

> >>  #endif

> >> -extern const pktio_if_ops_t tap_pktio_ops;

> >> -extern const pktio_if_ops_t * const pktio_if_ops[];

> >> +extern const HIDDEN pktio_if_ops_t tap_pktio_ops;

> >> +extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];

> >>

> >>  int sysfs_stats(pktio_entry_t *pktio_entry,

> >>                 odp_pktio_stats_t *stats);

> >> --

> >> 2.8.0.rc3

> >>

> >> _______________________________________________

> >> lng-odp mailing list

> >> lng-odp@lists.linaro.org

> >> https://lists.linaro.org/mailman/listinfo/lng-odp

> >

> >

> >

> >

> > --

> > Mike Holmes

> > Technical Manager - Linaro Networking Group

> > Linaro.org │ Open source software for ARM SoCs

> > "Work should be fun and collaborative, the rest follows"

> >

> >

>




-- 
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
"Work should be fun and collaborative, the rest follows"
Bill Fischofer April 11, 2016, 9:36 p.m. | #6
Agree with Mike.  These are effectively platform build issues, not general
API issues.

On Mon, Apr 11, 2016 at 3:45 PM, Mike Holmes <mike.holmes@linaro.org> wrote:

>

>

> On 11 April 2016 at 16:39, Anders Roxell <anders.roxell@linaro.org> wrote:

>

>> On 9 April 2016 at 20:07, Mike Holmes <mike.holmes@linaro.org> wrote:

>> >

>> >

>> > On 8 April 2016 at 17:14, Anders Roxell <anders.roxell@linaro.org>

>> wrote:

>> >>

>> >> Internal functions should not be part of symbols is visible outside the

>> >> library.

>> >>

>> >> Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>

>> >> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

>> >> ---

>> >>  .../linux-generic/include/odp_packet_io_internal.h | 23

>> >> +++++++++++++---------

>> >>  1 file changed, 14 insertions(+), 9 deletions(-)

>> >>

>> >> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h

>> >> b/platform/linux-generic/include/odp_packet_io_internal.h

>> >> index cca5c39..5c40c51 100644

>> >> --- a/platform/linux-generic/include/odp_packet_io_internal.h

>> >> +++ b/platform/linux-generic/include/odp_packet_io_internal.h

>> >> @@ -45,6 +45,11 @@ extern "C" {

>> >>   *  requested number of packets were not handled. */

>> >>  #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e !=

>> >> EINTR)

>> >>

>> >> +#if defined(__GNUC__)

>> >> +#  define HIDDEN  __attribute__((visibility("hidden")))

>> >

>> >

>> > Should this be in the compiler hints.h with the others ?

>>

>> either there or in in platform/linux-generic/include/odp_internal.h

>> since its an internal define.

>>

>

> I am ok with it being internal too, but then on principle I'd like to see

> the other internal hints removed from the public API files.

> At that point we will have reduced the public API surface area to only

> what it needs to contain.

>

>

>>

>> Cheers,

>> Anders

>>

>> >

>> > include/odp/api/spec/hints.h:#define ODP_NORETURN

>> > __attribute__((__noreturn__))

>> > include/odp/api/spec/hints.h:#define ODP_WEAK_SYMBOL

>> > __attribute__((__weak__))

>> > include/odp/api/spec/hints.h:#define ODP_HOT_CODE

>> > __attribute__((__hot__))

>> > include/odp/api/spec/hints.h:#define ODP_COLD_CODE

>> > __attribute__((__cold__))

>> >

>> > If it is part of the spec it is easier for other platforms to hide their

>> > internals by reusing it.

>> >

>> >

>> >>

>> >> +#else

>> >> +#  define HIDDEN

>> >> +#endif

>> >>  /* Forward declaration */

>> >>  struct pktio_if_ops;

>> >>

>> >> @@ -171,7 +176,7 @@ typedef struct pktio_if_ops {

>> >>  int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t

>> *base,

>> >>                         uint16_t buf_len, odp_packet_t *pkt_ret);

>> >>

>> >> -extern void *pktio_entry_ptr[];

>> >> +extern void HIDDEN *pktio_entry_ptr[];

>> >>

>> >>  static inline int pktio_to_id(odp_pktio_t pktio)

>> >>  {

>> >> @@ -217,16 +222,16 @@ int single_recv_queue(pktio_entry_t *entry, int

>> >> index, odp_packet_t packets[],

>> >>  int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t

>> >> packets[],

>> >>                       int num);

>> >>

>> >> -extern const pktio_if_ops_t netmap_pktio_ops;

>> >> -extern const pktio_if_ops_t dpdk_pktio_ops;

>> >> -extern const pktio_if_ops_t sock_mmsg_pktio_ops;

>> >> -extern const pktio_if_ops_t sock_mmap_pktio_ops;

>> >> -extern const pktio_if_ops_t loopback_pktio_ops;

>> >> +extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;

>> >> +extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;

>> >> +extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;

>> >> +extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;

>> >> +extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;

>> >>  #ifdef HAVE_PCAP

>> >> -extern const pktio_if_ops_t pcap_pktio_ops;

>> >> +extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;

>> >>  #endif

>> >> -extern const pktio_if_ops_t tap_pktio_ops;

>> >> -extern const pktio_if_ops_t * const pktio_if_ops[];

>> >> +extern const HIDDEN pktio_if_ops_t tap_pktio_ops;

>> >> +extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];

>> >>

>> >>  int sysfs_stats(pktio_entry_t *pktio_entry,

>> >>                 odp_pktio_stats_t *stats);

>> >> --

>> >> 2.8.0.rc3

>> >>

>> >> _______________________________________________

>> >> lng-odp mailing list

>> >> lng-odp@lists.linaro.org

>> >> https://lists.linaro.org/mailman/listinfo/lng-odp

>> >

>> >

>> >

>> >

>> > --

>> > Mike Holmes

>> > Technical Manager - Linaro Networking Group

>> > Linaro.org │ Open source software for ARM SoCs

>> > "Work should be fun and collaborative, the rest follows"

>> >

>> >

>>

>

>

>

> --

> Mike Holmes

> Technical Manager - Linaro Networking Group

> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs

> "Work should be fun and collaborative, the rest follows"

>

>

>

> _______________________________________________

> lng-odp mailing list

> lng-odp@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp

>

>
Maxim Uvarov April 12, 2016, 8:03 a.m. | #7
why you do this only for that functions? I think at the beginning of 
each internal.[ch] we need to say:

#pragma GCC visibility push(hidden)

then in the bottom:
#pragma GCC visibility pop

In that case all internal things will marked as .hidden and you don't 
need to specify it for each symbol.

Maxim.


On 04/09/16 00:14, Anders Roxell wrote:
> Internal functions should not be part of symbols is visible outside the
> library.
>
> Suggested-by: Ricardo Salveti <ricardo.salveti@linaro.org>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
>   .../linux-generic/include/odp_packet_io_internal.h | 23 +++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
> index cca5c39..5c40c51 100644
> --- a/platform/linux-generic/include/odp_packet_io_internal.h
> +++ b/platform/linux-generic/include/odp_packet_io_internal.h
> @@ -45,6 +45,11 @@ extern "C" {
>    *  requested number of packets were not handled. */
>   #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e != EINTR)
>   
> +#if defined(__GNUC__)
> +#  define HIDDEN  __attribute__((visibility("hidden")))
> +#else
> +#  define HIDDEN
> +#endif
>   /* Forward declaration */
>   struct pktio_if_ops;
>   
> @@ -171,7 +176,7 @@ typedef struct pktio_if_ops {
>   int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,
>   			uint16_t buf_len, odp_packet_t *pkt_ret);
>   
> -extern void *pktio_entry_ptr[];
> +extern void HIDDEN *pktio_entry_ptr[];
>   
>   static inline int pktio_to_id(odp_pktio_t pktio)
>   {
> @@ -217,16 +222,16 @@ int single_recv_queue(pktio_entry_t *entry, int index, odp_packet_t packets[],
>   int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t packets[],
>   		      int num);
>   
> -extern const pktio_if_ops_t netmap_pktio_ops;
> -extern const pktio_if_ops_t dpdk_pktio_ops;
> -extern const pktio_if_ops_t sock_mmsg_pktio_ops;
> -extern const pktio_if_ops_t sock_mmap_pktio_ops;
> -extern const pktio_if_ops_t loopback_pktio_ops;
> +extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;
> +extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;
> +extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;
> +extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;
> +extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;
>   #ifdef HAVE_PCAP
> -extern const pktio_if_ops_t pcap_pktio_ops;
> +extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;
>   #endif
> -extern const pktio_if_ops_t tap_pktio_ops;
> -extern const pktio_if_ops_t * const pktio_if_ops[];
> +extern const HIDDEN pktio_if_ops_t tap_pktio_ops;
> +extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];
>   
>   int sysfs_stats(pktio_entry_t *pktio_entry,
>   		odp_pktio_stats_t *stats);
Ricardo Salveti April 12, 2016, 9:41 p.m. | #8
On Tue, Apr 12, 2016 at 5:03 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> why you do this only for that functions? I think at the beginning of each
> internal.[ch] we need to say:
>
> #pragma GCC visibility push(hidden)
>
> then in the bottom:
> #pragma GCC visibility pop
>
> In that case all internal things will marked as .hidden and you don't need
> to specify it for each symbol.

At that point we went over the symbols we noticed when doing the
packaging, but indeed making it generic for every internal file is a
better option.

Cheers,

Ricardo

Patch

diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
index cca5c39..5c40c51 100644
--- a/platform/linux-generic/include/odp_packet_io_internal.h
+++ b/platform/linux-generic/include/odp_packet_io_internal.h
@@ -45,6 +45,11 @@  extern "C" {
  *  requested number of packets were not handled. */
 #define SOCK_ERR_REPORT(e) (e != EAGAIN && e != EWOULDBLOCK && e != EINTR)
 
+#if defined(__GNUC__)
+#  define HIDDEN  __attribute__((visibility("hidden")))
+#else
+#  define HIDDEN
+#endif
 /* Forward declaration */
 struct pktio_if_ops;
 
@@ -171,7 +176,7 @@  typedef struct pktio_if_ops {
 int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,
 			uint16_t buf_len, odp_packet_t *pkt_ret);
 
-extern void *pktio_entry_ptr[];
+extern void HIDDEN *pktio_entry_ptr[];
 
 static inline int pktio_to_id(odp_pktio_t pktio)
 {
@@ -217,16 +222,16 @@  int single_recv_queue(pktio_entry_t *entry, int index, odp_packet_t packets[],
 int single_send_queue(pktio_entry_t *entry, int index, odp_packet_t packets[],
 		      int num);
 
-extern const pktio_if_ops_t netmap_pktio_ops;
-extern const pktio_if_ops_t dpdk_pktio_ops;
-extern const pktio_if_ops_t sock_mmsg_pktio_ops;
-extern const pktio_if_ops_t sock_mmap_pktio_ops;
-extern const pktio_if_ops_t loopback_pktio_ops;
+extern const HIDDEN pktio_if_ops_t netmap_pktio_ops;
+extern const HIDDEN pktio_if_ops_t dpdk_pktio_ops;
+extern const HIDDEN pktio_if_ops_t sock_mmsg_pktio_ops;
+extern const HIDDEN pktio_if_ops_t sock_mmap_pktio_ops;
+extern const HIDDEN pktio_if_ops_t loopback_pktio_ops;
 #ifdef HAVE_PCAP
-extern const pktio_if_ops_t pcap_pktio_ops;
+extern const HIDDEN pktio_if_ops_t pcap_pktio_ops;
 #endif
-extern const pktio_if_ops_t tap_pktio_ops;
-extern const pktio_if_ops_t * const pktio_if_ops[];
+extern const HIDDEN pktio_if_ops_t tap_pktio_ops;
+extern const HIDDEN pktio_if_ops_t * const pktio_if_ops[];
 
 int sysfs_stats(pktio_entry_t *pktio_entry,
 		odp_pktio_stats_t *stats);