hb-algs.hh(352) : warning C4172: returning address of local variable or temporary

[dejanyy]

When compiling the latest HarfBuzz (2.6.4) MS Visual C++ 15 gives the following 3 warnings:

hb-algs.hh(352) : warning C4172: returning address of local variable or temporary
hb-algs.hh(359) : warning C4172: returning address of local variable or temporary
hb-algs.hh(366) : warning C4172: returning address of local variable or temporary

The relevant code snippet is below. Lines 352, 359 and 366 are in bold:

/* Note.  In min/max impl, we can use hb_type_identity<T> for second argument.
 * However, that would silently convert between different-signedness integers.
 * Instead we accept two different types, such that compiler can err if
 * comparing integers of different signedness. */
struct
{
  template <typename T, typename T2> constexpr auto
  **operator () (T&& a, T2&& b) const HB_AUTO_RETURN <!-- THIS IS LINE 352**
  (hb_forward<T> (a) <= hb_forward<T2> (b) ? hb_forward<T> (a) : hb_forward<T2> (b))
}
HB_FUNCOBJ (hb_min);
struct
{
  template <typename T, typename T2> constexpr auto
  **operator () (T&& a, T2&& b) const HB_AUTO_RETURN <!-- THIS IS LINE 359**
  (hb_forward<T> (a) >= hb_forward<T2> (b) ? hb_forward<T> (a) : hb_forward<T2> (b))
}
HB_FUNCOBJ (hb_max);
struct
{
  template <typename T, typename T2, typename T3> constexpr auto
  **operator () (T&& x, T2&& min, T3&& max) const HB_AUTO_RETURN <!-- THIS IS LINE 366**
  (hb_min (hb_max (hb_forward<T> (x), hb_forward<T2> (min)), hb_forward<T3> (max)))
}
HB_FUNCOBJ (hb_clamp);

However, I do not understand what this code does.

Is this a known warning? It doesn't look like it should be ignored -- returning an address of a local variable or temporary probably results in undefined behaviour. Not sure...

[behdad]

I'll be damned. Let me see if I can figure out how this can be fixed...

I wonder if it's wrong to forward same argument twice to begin with...

At any rate, I don't understand how auto can decide to return an lvalue when the statement is rvalue. Basically what I'm saying is with HB_AUTO_RETURN this is impossible and definitely a compiler bug.

Would love to see someone who knows better chime in.

[behdad]

Also, unless you get same warning with their latest compiler, I'd just ignore this.

[dejanyy]

I actually did get this warning with the latest and greatest MS Visual Studio 2017 (Version 15.9.21). I actually updated my Visual Studio a few days ago specifically because of this warning (I wanted to see it this is perhaps a compiler bug). But the warning remains...

As I said, I don't really understand what this code does. Hopefully somebody can chime in. Or if you are confident this is a compiler bug, so be it...

[ebraminio]

Happens with VS2017 https://ci.appveyor.com/project/harfbuzz/harfbuzz/builds/31899255/job/2q148ic0mjvld8c5#L1478 and VS2019 https://ci.appveyor.com/project/harfbuzz/harfbuzz/builds/31899255/job/5304e8ijxhheyi0x#L1502 and apparently the check weren't implemented in VS2015.

[CoffeeFlux]

If you can get some sort of minimal repro for the bug, I'm happy to pass it on to the VC++ team; would be nice to see this cleaned up.

Also, you might want to edit that snippet back to something like operator () (T&& a, T2&& b) const HB_AUTO_RETURN // THIS IS LINE 352, as-is the leading ** is somewhat confusing.

[behdad]

That's super helpful. Thanks.

[CoffeeFlux]

A friend spotted the issue: you have excessive parens around the return, which means it's returning a reference. You might just have the warning disabled on other compilers? See https://godbolt.org/z/PqEbdK

Edit: nvm, see below

[behdad]

Oof. That' brutal!

THANK YOU! I'll see what to do with it.

[randrew]

Hi, I'm the friend. In this case it will happen even if you remove the parens. It could also happen if you left the parens and removed other stuff. Here's another simplified case where it will be wrong: https://godbolt.org/z/aWxbWo

Update: actually, we aren't entirely sure what's going on with MSVC.

Update 2: more information in the next comment.

[ihameed]

There are two problems, I think:

1. hb_min, hb_max, and hb_clamp are almost always called with rvalue parameters. The definition of hb_max is this:

   struct {
       template <typename T, typename T2>
       constexpr auto
       operator () (T && a, T2 && b) const
           -> decltype (hb_forward<T> (a) >= hb_forward<T2> (b) ? hb_forward<T> (a) : hb_forward<T2> (b))
       {
           return hb_forward<T> (a) >= hb_forward<T2> (b) ? hb_forward<T> (a) : hb_forward<T2> (b);
       }
   } const hb_max;

   When hb_max is called with two rvalues of type int, T is deduced to be int, T2 is deduced to be int, the expression (hb_forward<T> (a) <= hb_forward<T2> (b) ? hb_forward<T> (a) : hb_forward<T2> (b)) has type int &&, and the type of hb_max::operator () is then int && (int && a, int && b).

   One expression that triggers this warning can be found in hb-buffer-serialize.cc:

   harfbuzz/src/hb-buffer-serialize.cc

   Line 145 in 5539d15

   p += hb_max (0, snprintf (p, ARRAY_LENGTH (b) - (p - b), "%u", info[i].codepoint));

   When hb_max (...) is evaluated here, two temporaries of type int are created: one contains the constant 0 and the other contains the return value of snprintf. Two rvalue references are then bound to these temporaries and are passed to hb_max. N3337 12.2.5 (I don't know if the rules have changed since C++11) says:

   A temporary bound to a reference parameter in a function call (5.2.2) persists until the completion of the full-expression containing the call.

   So I think MSVC is wrong in issuing a warning for this; these temporaries have lifetimes that are specified to persist beyond the end of the call to hb_max::operator ().

   Here's a small repro:

   #include <utility> // std::forward
   
   int &&
   hb_max_int(int &&a, int &&b) {
       return a >= b ? std::forward<int>(a) : std::forward<int>(b);
   };
   
   void
   fake_hb_buffer_serialize_glyphs_json() {
       int ok = 0;
       ok += hb_max_int(0, 1);
   }

2. When building harfbuzz, MSVC deduces that hb_max::operator (), when applied to two int &&s, returns an int &&. But in a standalone source file, MSVC deduces that the return type of an identical construct is int.

   Here's a small repro:

   #include <utility>
   
   template <typename t>
   struct dump;
   
   struct {
       template <typename T, typename T2> constexpr auto
       operator () (T && a, T2 && b) const
           -> decltype
           (std::forward<T> (a) >= std::forward<T2> (b) ? std::forward<T> (a) : std::forward<T2> (b))
       {
           dump<decltype(std::forward<T> (a) >= std::forward<T2> (b) ? std::forward<T> (a) : std::forward<T2> (b))> _;
           return std::forward<T> (a) >= std::forward<T2> (b) ? std::forward<T> (a) : std::forward<T2> (b);
       }
   } const hb_max;
   
   void
   fake_hb_buffer_serialize_glyphs_json() {
       int ok = 0;
       ok += hb_max(5, 16);
   }

   Clang and gcc both think that int && is the type of this ternary expression, while MSVC thinks it has type int. But if you add a template <typename t> struct dump; to hb-buffer-serialize.c and then attempt to instantate one inside _hb_buffer_serialize_glyphs_json like so:

   dump<decltype(hb_max (0, snprintf (p, ARRAY_LENGTH (b) - (p - b), "%u", info[i].codepoint)))> asdf;
   

   You'll get this error when building:

   ...\harfbuzz\src\hb-buffer-serialize.cc(149): error C2079: 'asdf' uses undefined struct 'dump<T &&>'
           with
           [
               T=int
           ]
   

   I have no idea why MSVC is behaving differently when compiling a small repro vs compiling the entirety of harfbuzz, and I'm disinclined to spend a lot of time trying to narrow this down. Clang and GCC, in contrast, consistently determine that the return type of hb_max::operator () is int &&.

I wonder if it's wrong to forward same argument twice to begin with...

There is a subtle issue. Take hb_max as an example. When hb_max is applied to two rvalues, the hb_forwards in hb_max have the effect of passing a T && and T2 && to operator >=, instead of a T & and T2 & as would have happend otherwise. If you had an implementation of operator >= that took its parameters by value, and the type you're forwarding has a move constructor that has "affine" behavior (that leaves the source in a "valid, but unspecified" state), then you'd return a reference to one of two "valid, but unspecified" values in the body of the ternary expression. You'd want a >= b ? hb_forward<T>(a) : hb_forward<T2>(b) instead, at least.

Another perspective: hb_min, hb_max, and hb_clamp are only applied to small, cheaply duplicatable machine integers and machine floats in harfbuzz. There doesn't seem to be a real, practical reason to use this rvalue reference/forwarding stuff for these functions. All it does is make your code compile slower.

At any rate, I don't understand how auto can decide to return an lvalue when the statement is rvalue. Basically what I'm saying is with HB_AUTO_RETURN this is impossible and definitely a compiler bug.

An rvalue reference is still a mutable reference that may appear on the left hand side of an assignment.

[CoffeeFlux]

The first part has been fixed, pending a release.

[CoffeeFlux]

Oh, forgot to update again. First issue: https://developercommunity.visualstudio.com/content/problem/1225750/msvc-calculates-lifetime-for-temporary-incorrectly.html

This is pending a public release with the fix.

Second issue: https://developercommunity.visualstudio.com/content/problem/1225892/msvc-incorrectly-detects-return-type-with-template.html

The fix here is to pass /Zc:ternary when compiling on Windows, as by default the compiler does not conform to the standard for legacy reasons. More detailed info can be found at https://docs.microsoft.com/en-us/cpp/build/reference/zc-ternary

[behdad]

you have excessive parens around the return, which means it's returning a reference.

I inspected that:

#define HB_AUTO_RETURN(E) -> decltype ((E)) { return (E); }

I believe the parens are expected.

I wonder if it's wrong to forward same argument twice to begin with...

Fixed this.

Another perspective: hb_min, hb_max, and hb_clamp are only applied to small, cheaply duplicatable machine integers and machine floats in harfbuzz. There doesn't seem to be a real, practical reason to use this rvalue reference/forwarding stuff for these functions. All it does is make your code compile slower.

True. I prefer to write our generic algorithms as generics though. That said, we're getting to a point that I like to just start using std instead.

Anyway. Anything else I need to be fixing here, @ihameed @randrew @CoffeeFlux ?

[randrew]

Well, it seems fine to me. But, I guessed the original cause incorrectly, anyway, so what do I know :)

I hadn't expected the ternary compatibility thing to affect a return type. I had thought it only affected compilations being accepted or rejected. The comments from the MSFT engineers hint they will update the documentation to reflect that.

[behdad]

Is there any way to turn on/off pragmas from within the code to get the correct behavior?

[CoffeeFlux]

I think you can just enable /Zc:ternary globally for the project, not sure there's a reason to rely on the non-standard behavior. I'm not aware of a way to do it inline.

[behdad]

Anyone who likes to try adding that to any of our build systems please do so.

[CoffeeFlux]

16.9 has been promoted to stable, which includes the lifetime fix: https://developercommunity2.visualstudio.com/t/msvc-calculates-lifetime-for-temporary-incorrectly/1225750