[llvm] 48c74bb - [SampleProfileInference] Work around odr-use of const non-inline static data member to fix -O0 builds after D120508

[David Blaikie via llvm-commits]

On Mon, Jun 27, 2022 at 11:28 PM Fangrui Song <i at maskray.me> wrote:
>
> On Mon, Jun 27, 2022 at 10:45 PM David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Jun 27, 2022, 9:23 PM Fangrui Song <i at maskray.me> wrote:
> >>
> >> On Mon, Jun 27, 2022 at 6:01 PM David Blaikie <dblaikie at gmail.com> wrote:
> >> >
> >> > Sorry, didn't see this earlier, but came across it looking at a
> >> > similar change recently - could you add a definition of this instead
> >> > of a cast, so it's not a trip-hazard for future development?
> >>
> >> Do you mean something like D.6 Redeclaration of static constexpr data
> >> members [depr.static.constexpr] ?
> >> The usage is deprecated from C++17 onwards.
> >
> >
> > Yep, that. So long as we're building in C++14 mode that's the suitable thing to do. In 17 onwards we can remove them without them being a trap for new code to trip over like this.
>
> So my plan is to leave them as is. When llvm-project moves to C++17,
> remove the casts.
> For now, it seems that the deprecated redundant declaration
> (definition in C++14) does not cause a GCC/Clang warning, but I can't
> say that it will be future proof.

But the current code isn't present-proof. It's pretty easy to odr-use
variables like this (eg: passing them through any forwarding API like
std::function).

I think we should design/write this for the language we have today -
it'll be easy to remove the redundant declarations later (especially
if there's a warning that finds them) compared to removing quirky
casts that won't be identified by any tooling. And we'll likely have a
bunch of these to cleanup beyond the few we might avoid by using casts
instead of definitions.

> I understand that providing a redundant declaration will allow odr-use
> in more than one places, but such std::min/std::max occurrences seem
> rare enough for concern in practice.
>
> >> >
> >> > On Tue, Mar 8, 2022 at 2:35 PM Fangrui Song via llvm-commits
> >> > <llvm-commits at lists.llvm.org> wrote:
> >> > >
> >> > >
> >> > > Author: Fangrui Song
> >> > > Date: 2022-03-08T14:34:53-08:00
> >> > > New Revision: 48c74bb2e2a72830f1068823bfc2f6fd4b53d427
> >> > >
> >> > > URL: https://github.com/llvm/llvm-project/commit/48c74bb2e2a72830f1068823bfc2f6fd4b53d427
> >> > > DIFF: https://github.com/llvm/llvm-project/commit/48c74bb2e2a72830f1068823bfc2f6fd4b53d427.diff
> >> > >
> >> > > LOG: [SampleProfileInference] Work around odr-use of const non-inline static data member to fix -O0 builds after D120508
> >> > >
> >> > > MinBaseDistance may be odr-used by std::max, leading to an undefined symbol linker error:
> >> > >
> >> > > ```
> >> > > ld.lld: error: undefined symbol: (anonymous namespace)::MinCostMaxFlow::MinBaseDistance
> >> > > >>> referenced by SampleProfileInference.cpp:744 (/home/ray/llvm-project/llvm/lib/Transforms/Utils/SampleProfileInference.cpp:744)
> >> > > >>>               lib/Transforms/Utils/CMakeFiles/LLVMTransformUtils.dir/SampleProfileInference.cpp.o:((anonymous namespace)::FlowAdjuster::jumpDistance(llvm::FlowJump*) const)
> >> > > ```
> >> > >
> >> > > Since llvm-project is still using C++ 14, workaround it with a cast.
> >> > >
> >> > > Added:
> >> > >
> >> > >
> >> > > Modified:
> >> > >     llvm/lib/Transforms/Utils/SampleProfileInference.cpp
> >> > >
> >> > > Removed:
> >> > >
> >> > >
> >> > >
> >> > > ################################################################################
> >> > > diff  --git a/llvm/lib/Transforms/Utils/SampleProfileInference.cpp b/llvm/lib/Transforms/Utils/SampleProfileInference.cpp
> >> > > index 298c36b7ecd3d..6f56f1c432c2e 100644
> >> > > --- a/llvm/lib/Transforms/Utils/SampleProfileInference.cpp
> >> > > +++ b/llvm/lib/Transforms/Utils/SampleProfileInference.cpp
> >> > > @@ -741,7 +741,7 @@ class FlowAdjuster {
> >> > >    /// parts to a multiple of 1 / BaseDistance.
> >> > >    int64_t jumpDistance(FlowJump *Jump) const {
> >> > >      uint64_t BaseDistance =
> >> > > -        std::max(MinCostMaxFlow::MinBaseDistance,
> >> > > +        std::max(static_cast<uint64_t>(MinCostMaxFlow::MinBaseDistance),
> >> > >                   std::min(Func.Blocks[Func.Entry].Flow,
> >> > >                            MinCostMaxFlow::AuxCostUnlikely / NumBlocks()));
> >> > >      if (Jump->IsUnlikely)
> >> > >
> >> > >
> >> > >
> >> > > _______________________________________________
> >> > > llvm-commits mailing list
> >> > > llvm-commits at lists.llvm.org
> >> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits