Re: Help with fixing clang-diagnostic-over-aligned

[Sailesh Mukil]

Thanks for the explanation Todd and Dan! I had introduced a new
CACHELINE_ALIGNED object into the class which caused this problem. Making
the parent classes CACHELINE_ALIGNED too fixed the problem.

On Wed, Feb 14, 2018 at 12:58 PM, Todd Lipcon  wrote:

> This probably means that QueryExecMgr (or one of its descendent members) is
> annotated with __attribute__((aligned(64)), perhaps through something like
> a CACHELINE_ALIGNED macro.
>
> The issue is that, as the warning says, normal libc malloc doesn't make any
> hard guarantees about the alignment of allocations, and the way that
> 'operator new' is implemented by default only gets the size and not the
> desired allocation. I believe C++17 is adding support for an 'operator new'
> overload that also passes the desired alignment, so that it can call down
> into posix_memalign and ensure the appropriate allocation.
>
> The "correct" fix prior to C++17 is probably to do something like overload
> 'operator new' for the type, or to use posix_memalign manually to allocate
> aligned memory, and then placement-new it into that memory.
>
> Another reasonable fix would probably be to add a CHECK in the constructor
> that 'this' is aligned as desired so that, if you ever run on an allocator
> that isn't guaranteeing what you think it is, you'll know immediately
> rather than getting a surprising perf or atomicity regression due to
> cross-cacheline loads/stores.
>
> -Todd
>
>
> On Wed, Feb 14, 2018 at 12:33 PM, Sailesh Mukil 
> wrote:
>
> > Does anyone have experience with fixing the following type of
> > clang-tidy warning?
> >
> >
> > /home/ubuntu/Impala/be/src/runtime/exec-env.cc:159:21: warning: type
> > 'impala::QueryExecMgr' requires 64 bytes of alignment and the default
> > allocator only guarantees 16 bytes [clang-diagnostic-over-aligned]
> > query_exec_mgr_(new QueryExecMgr()),
> >
> >
> > /home/ubuntu/Impala/be/src/service/impalad-main.cc:80:49: warning:
> > type 'impala::ImpalaServer' requires 64 bytes of alignment and the
> > default allocator only guarantees 16 bytes
> > [clang-diagnostic-over-aligned]
> >   boost::shared_ptr impala_server(new
> > ImpalaServer(_env));
> >
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

Re: Help with fixing clang-diagnostic-over-aligned

[Todd Lipcon]

This probably means that QueryExecMgr (or one of its descendent members) is
annotated with __attribute__((aligned(64)), perhaps through something like
a CACHELINE_ALIGNED macro.

The issue is that, as the warning says, normal libc malloc doesn't make any
hard guarantees about the alignment of allocations, and the way that
'operator new' is implemented by default only gets the size and not the
desired allocation. I believe C++17 is adding support for an 'operator new'
overload that also passes the desired alignment, so that it can call down
into posix_memalign and ensure the appropriate allocation.

The "correct" fix prior to C++17 is probably to do something like overload
'operator new' for the type, or to use posix_memalign manually to allocate
aligned memory, and then placement-new it into that memory.

Another reasonable fix would probably be to add a CHECK in the constructor
that 'this' is aligned as desired so that, if you ever run on an allocator
that isn't guaranteeing what you think it is, you'll know immediately
rather than getting a surprising perf or atomicity regression due to
cross-cacheline loads/stores.

-Todd


On Wed, Feb 14, 2018 at 12:33 PM, Sailesh Mukil 
wrote:

> Does anyone have experience with fixing the following type of
> clang-tidy warning?
>
>
> /home/ubuntu/Impala/be/src/runtime/exec-env.cc:159:21: warning: type
> 'impala::QueryExecMgr' requires 64 bytes of alignment and the default
> allocator only guarantees 16 bytes [clang-diagnostic-over-aligned]
> query_exec_mgr_(new QueryExecMgr()),
>
>
> /home/ubuntu/Impala/be/src/service/impalad-main.cc:80:49: warning:
> type 'impala::ImpalaServer' requires 64 bytes of alignment and the
> default allocator only guarantees 16 bytes
> [clang-diagnostic-over-aligned]
>   boost::shared_ptr impala_server(new
> ImpalaServer(_env));
>



-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Help with fixing clang-diagnostic-over-aligned

[Daniel Hecht]

Anything that has an aligned field will also need to be aligned (otherwise
the field alignment won't be respected).  You can do it by inheriting from
CacheLinedAlign which includes an operator new (see comment in
aligned-new.h).

On Wed, Feb 14, 2018 at 12:33 PM, Sailesh Mukil 
wrote:

> Does anyone have experience with fixing the following type of
> clang-tidy warning?
>
>
> /home/ubuntu/Impala/be/src/runtime/exec-env.cc:159:21: warning: type
> 'impala::QueryExecMgr' requires 64 bytes of alignment and the default
> allocator only guarantees 16 bytes [clang-diagnostic-over-aligned]
> query_exec_mgr_(new QueryExecMgr()),
>
>
> /home/ubuntu/Impala/be/src/service/impalad-main.cc:80:49: warning:
> type 'impala::ImpalaServer' requires 64 bytes of alignment and the
> default allocator only guarantees 16 bytes
> [clang-diagnostic-over-aligned]
>   boost::shared_ptr impala_server(new
> ImpalaServer(_env));
>

Help with fixing clang-diagnostic-over-aligned

[Sailesh Mukil]

Does anyone have experience with fixing the following type of
clang-tidy warning?


/home/ubuntu/Impala/be/src/runtime/exec-env.cc:159:21: warning: type
'impala::QueryExecMgr' requires 64 bytes of alignment and the default
allocator only guarantees 16 bytes [clang-diagnostic-over-aligned]
query_exec_mgr_(new QueryExecMgr()),


/home/ubuntu/Impala/be/src/service/impalad-main.cc:80:49: warning:
type 'impala::ImpalaServer' requires 64 bytes of alignment and the
default allocator only guarantees 16 bytes
[clang-diagnostic-over-aligned]
  boost::shared_ptr impala_server(new ImpalaServer(_env));