This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Improve diagnostic about addr spaces for overload candidates
ClosedPublic

Authored by Anastasia on Dec 6 2019, 4:50 AM.

Details

Summary

As discussed in https://reviews.llvm.org/D69938#inline-629726 this commit improves the diagnostic of addr spaces. The approach is currently reusing diagnostic streaming of Qualifiers.

There are a number of issues however:

  1. Address spaces don't always have representation i.e. in C++ it is just a number or Default address space doesn't correspond to anything at all. For the former case with the current approach we will get '__attribute__((address_space(N)))' printed into the diagnostic. For the latter one it will print unqualified.
  1. In OpenCL we threat __private and Default in the same way and therefore it doesn't get printed. With this patch therefore unqualified will appear even if __private was specified in the original source. There is a bug open to fix that however: https://bugs.llvm.org/show_bug.cgi?id=43295. Perhaps it should be fixed as soon as this gets committed.

Diff Detail

Event Timeline

Anastasia created this revision.Dec 6 2019, 4:50 AM

Hmm. How about:

// For the `this` argument
candidate function not viable: 'this' object is in '__private' address space, but method expects object in '__global' address space

// For pointer arguments
candidate function not viable: cannot pass pointer to '__private' address space as a pointer to '__global' address space in 1st argument

// For reference arguments
candidate function not viable: cannot bind reference in '__global' address space to object in '__private' address space in 1st argument

This would require you to render a string describing the address space yourself. I would suggest "generic address space" for the default AS outside of OpenCL, "'foo' address space" for a language-specific AS (including implicit __private in OpenCL), or "address space N" for a numbered address space.

What do you think?

  • Allow sending address spaces into diagnostics
  • Change wording of diagnostics for address spaces in overloading
rjmccall added inline comments.Dec 11 2019, 12:59 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3996

I think you need to move "address space" into the diagnostic rendering for ak_addrspace so that you can say "generic address space" instead of "address space generic".

clang/lib/AST/TypePrinter.cpp
1777

Can you just make these two cases in the switch? opencl_private already seems to be there.

Anastasia updated this revision to Diff 233579.Dec 12 2019, 4:19 AM
Anastasia marked an inline comment as done.
  • Moved "address space" printing into diagnostic engine
  • Moved LangAS::Default into switch/case statement.
rjmccall accepted this revision.Dec 12 2019, 9:51 AM

Thanks, that looks much better.

This revision is now accepted and ready to land.Dec 12 2019, 9:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 4:36 AM

Hi,

I am investigating a recurring regression on aarch64-linux bots and bisecting the commits on the build [1] that introduced the regression it points to this one. I don't understand exactly what is triggering the issue, but it only happen on the stage2 build with stage1 clang built with different compilers (gcc 5.4, gcc-7.4.0, and gcc-8.3.0). The issue is on clang itself while trying to execute the Analysis/uninit-sometimes.cpp test:

******************** TEST 'Clang :: Analysis/uninit-sometimes.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 -internal-isystem /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized -verify /home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp
: 'RUN: at line 2';   /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 -internal-isystem /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized -fdiagnostics-parseable-fixits /home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp 2>&1 | /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/FileCheck /home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp
--
Exit Code: 134

Command Output (stderr):
--
clang: /home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/lib/Basic/Diagnostic.cpp:591: void HandleSelectModifier(const clang::Diagnostic &, unsigned int, const char *, unsigned int, SmallVectorImpl<char> &): Assertion `NextVal != ArgumentEnd && "Value for integer select modifier was" " larger than the number of options in the diagnostic string!"' failed.
Stack dump:
0.	Program arguments: /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 -internal-isystem /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized -verify /home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp 
1.	/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp:161:1: current parser token 'int'
2.	/home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp:146:32: parsing function body 'test_for_range_true'
#0 0x000000000182be04 PrintStackTraceSignalHandler(void*) (/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang+0x182be04)
/home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/tools/clang/test/Analysis/Output/uninit-sometimes.cpp.script: line 2: 63297 Aborted                 (core dumped) /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/bin/clang -cc1 -internal-isystem /home/buildslave/buildslave/clang-cmake-aarch64-full/stage2/lib/clang/10.0.0/include -nostdsysteminc -std=gnu++11 -Wsometimes-uninitialized -verify /home/buildslave/buildslave/clang-cmake-aarch64-full/llvm/clang/test/Analysis/uninit-sometimes.cpp

The bots run a ubuntu 16.04 container and I can't reproduce on a different one, I am still trying to come up with a better reproduce case. Also, moving the both DiagnosticBuilder::AddString and DiagnosticBuilder::AddTaggedVal outside clang/include/clang/Basic/Diagnostic.h to clang/lib/Basic/Diagnostic.cpp "fixes" the issue (indicating the something is wrong at code generation). This issue has been shown for some time on aarch64-linux bot [2]. Any idea if this is an environmental issue or a real issue?

[1] http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/8562
[2] http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full