Remove usage of __builtin_constant_p under IBM XL

[AndrewGaspar]

Description

This change eliminates use of the __builtin_constant_p intrinsic for IBM XL compilers that use the clang front end. Under XL, code is generated to call destructors for temporaries in expressions passed to __builtin_constant_p, but the code to initialize those temporaries is not emitted. For example, IBM XL will generate a call to std::string::~string for the following code:

__builtin_constant_p(std::string("12") + "34");

This often results in a segfault in the case of std::string::~string, as free will likely be called on an invalid address.

Concretely, this code would result in a segfault:

REQUIRE(std::string("12") + "34" == "1234");

Previously, this the tests would fail when building with IBM XL 16.1.1.7. Now they all pass.

The bug has been reported to IBM. I will re-enable this for future versions of IBM XL when the problem is fixed.

[codecov]

Codecov Report

Merging #1907 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1907   +/-   ##
=======================================
  Coverage   86.49%   86.49%           
=======================================
  Files         131      131           
  Lines        3900     3900           
=======================================
  Hits         3373     3373           
  Misses        527      527           

[horenmar]

Under XL, code is generated to call destructors for temporaries in expressions passed to __builtin_constant_p, but the code to initialize those temporaries is not emitted.

🤦

It shouldn't evaluate the expression at all, it is just an unevaluated context that allows lambdas (unlike sizeof). Anyway, thanks for the fix.

[AndrewGaspar]

Don't touch your face! 🙂

I didn't actually realize how recently this issue was introduced.
Thanks for merging quickly!

[cebowler]

I've noted that the clang compiler has a similar problem. You may want to check clang instead (which the XL compiler also defines) just in case you ever use that compiler. I'd also suggest opening a bugzilla report with LLVM.

[AndrewGaspar]

Oh, I had assumed that this had been tested in clang. I should have checked.

[AndrewGaspar]

@cebowler I can't repro this on either Apple's Clang distribution or Clang 9.0. Using this code:

#include <iostream>

class Foo {
public:
    Foo() {
        std::cout << "Foo::Foo" << std::endl;
    }

    ~Foo() {
        std::cout << "Foo::~Foo" << std::endl;
    }
};

int main(int argc, char *argv[]) {
    __builtin_constant_p(Foo());
}

If clang exhibited the bug, I would expect to see the following output:

Foo::~Foo

[AndrewGaspar]

Tried it on a Power9 system with clang 7 and clang 8 and couldn't reproduce the issue. Only with an XL compiler.

[hubert-reinterpretcast]

Thanks for checking. The similar Clang issues that have been reported have also been fixed:
llvm/llvm-project@4b03dd7
llvm/llvm-project@e128f71

[AndrewGaspar]

Must have been introduced fairly recently - didn't have a clang 10.0 or nightly build lying around to test against. Our IBM XL compiler is fairly recent, so not surprising it would have picked up that issue.

[horenmar]

As far as I can see, the linked issues are about when __builtin_constant_p evaluates to true, but not about what happens with the code inside.

[hubert-reinterpretcast]

As far as I can see, the linked issues are about when __builtin_constant_p evaluates to true, but not about what happens with the code inside.

That's not what I see. Please refer to the linked bug report: https://llvm.org/PR45535.

[hubert-reinterpretcast]

That said, the implementation in XL does not act exactly the same as in Clang.

[horenmar]

@hubert-reinterpretcast Well, I stand corrected.

Anyway, I would welcome a PR for specific version when IBM XL fixes this.

---

As for Clang, it currently seems to work properly, so until bug reports with specific version come in, I am going to keep the __builtin_constant_p enabled unconditionally.

[AndrewGaspar]

btw, IBM has fixed this in a recent compiler release, at least to us. At some point this could be re-enabled for certain IBM XL versions if somebody is so inclined.

[horenmar]

Yeah, I am open to a follow-up PR, but I don't intend to get hold of IBM XL to test it myself.