Page MenuHomePhabricator

[TargetLoweringObjectFileImpl] Use llvm::transform
ClosedPublic

Authored by orivej on Sat, May 23, 8:33 AM.

Details

Summary

Fixes a build issue with libc++ configured with _LIBCPP_RAW_ITERATORS (ADL not effective)

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1602:3: error: no matching function for call to 'transform'
  transform(HexString.begin(), HexString.end(), HexString.begin(), tolower);
  ^~~~~~~~~

Diff Detail

Event Timeline

orivej created this revision.Sat, May 23, 8:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, May 23, 8:33 AM

Which compiler does you use? The file misses <algorithm>

You can also use llvm::transform

orivej updated this revision to Diff 265879.Sat, May 23, 11:58 AM
orivej retitled this revision from [TargetLoweringObjectFileImpl] Qualify std::transform to [TargetLoweringObjectFileImpl] Use llvm::transform.
orivej added a comment.EditedSat, May 23, 12:11 PM

I have reproduced the error with Clang 7 and 10. I am compiling with a slightly customized version of a fresh libcxx.

The interesting question is why you or CI can build llvm without this error. What brings std::transform into scope?

MaskRay added a comment.EditedSat, May 23, 2:35 PM

I have reproduced the error with Clang 7 and 10. I am compiling with a slightly customized version of a fresh libcxx.

The interesting question is why you or CI can build llvm without this error. What brings std::transform into scope?

I cannot reproduce with clang 7. My command line:

/home/ray/ccls/build/clang+llvm-7.0.0-x86_64-linux-gnu-ubuntu-16.04/bin/clang -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/CodeGen -I/home/ray/llvm/llvm/lib/CodeGen -I/usr/include/libxml2 -Iinclude -I/home/ray/llvm/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -g -fPIC -fno-exceptions -fno-rtti -gsplit-dwarf -std=c++14 -c /home/ray/llvm/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp -fsyntax-only

You can delete TargetLoweringObjectFileImpl.cpp.o and run ninja -v llc to get the clang command line. By checking -H, I find that <algorithm> is included via llvm/ADT/Hashing.h

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1749

Use llvm::transform to avoid future ADL gotcha.

orivej marked an inline comment as done.EditedSat, May 23, 3:24 PM

My C++ library does not allow even this:

#include <algorithm>
#include <string>

int main() {
    std::string s("hello");
    transform(s.begin(), s.end(), s.begin(), [](char c) -> char { return c; });
}
main.cpp:7:5: error: use of undeclared identifier 'transform'; did you mean 'std::transform'?
    transform(s.begin(), s.end(), s.begin(), [](char c) -> char { return c; });
    ^~~~~~~~~
    std::transform
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1749

Are you sure? Changing the signature like this makes the name resolution argument independent, since llvm::transform is in scope due to using namespace llvm;. Nothing else in this file qualifies llvm::.

My C++ library does not allow even this:

Can you double check?? The code below even works with clang 3.4 and GCC 4.7, which are both very old (lower than LLVM's build requirement).
Probably not libstdc++ or libc++.. Sigh

#include <algorithm>
#include <string>

int main() {
    std::string s("hello");
    transform(s.begin(), s.end(), s.begin(), [](char c) -> char { return c; });
}
main.cpp:7:5: error: use of undeclared identifier 'transform'; did you mean 'std::transform'?
    transform(s.begin(), s.end(), s.begin(), [](char c) -> char { return c; });
    ^~~~~~~~~
    std::transform
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1749

We prefer a qualified name. This is similar to llvm::sort. Unqualified name lookups can cause ADL. STLExtras.h defines names the same as in std::. A misuse may easily pick up an unintended overload in std::

orivej updated this revision to Diff 265956.Sun, May 24, 6:54 PM

I suppose that the newer versions are generally more restrictive than the older ones. Old libstdc++ used to bring std names into the global scope.

I have found what causes the build failure on my part: it is that string.begin() and end() return a char *, not an iterator class in the std namespace: https://github.com/catboost/catboost/blob/08a65a2e/contrib/libs/cxxsupp/libcxx/include/string#L697

Could you please commit on my behalf?

The char * iterator feature could have been enabled in libcxx via _LIBCPP_RAW_ITERATORS config definition until https://reviews.llvm.org/D79323.

MaskRay accepted this revision.Sun, May 24, 8:58 PM

Will do!

This revision is now accepted and ready to land.Sun, May 24, 8:58 PM
MaskRay edited the summary of this revision. (Show Details)Sun, May 24, 8:59 PM
MaskRay added a comment.EditedSun, May 24, 9:02 PM

I suppose that the newer versions are generally more restrictive than the older ones. Old libstdc++ used to bring std names into the global scope.

If string::begin returns a std::string::iterator (everywhere else in the world...), names in std:: are looked up ("The innermost enclosing namespaces in the classes added to the set") when you call an unqualified transform.

If string::begin returns const char *, ADL does not fire and std:: is not looked up.

This revision was automatically updated to reflect the committed changes.