clangd / vscode-clangd

Visual Studio Code extension for clangd
https://marketplace.visualstudio.com/items?itemName=llvm-vs-code-extensions.vscode-clangd
MIT License
629 stars 106 forks source link

cland wrongly recognise "thread_local" as "__thread" in my projcet #577

Closed coyorkdow closed 7 months ago

coyorkdow commented 9 months ago

Please describe the problem. For hints on what information is helpful, see: https://clangd.llvm.org/troubleshooting.html

I have some variables within the function scope, and they have the thread_local storage. I use thread_local to specify them.

However, cland thought that I used __thread, and give me an error message (the following screenshot). 20240104-145950

Logs Please attach the clangd log from the "Output" window if you can. If possible, run with --log=verbose - note that the logs will include the contents of open files!

System information

clangd version 17.0.3 (https://github.com/llvm/llvm-project 888437e1b60011b8a375dd30928ec925b448da57) Features: linux+grpc Platform: x86_64-unknown-linux-gnu

v0.1.26

PRETTY_NAME="Debian GNU/Linux 10 (buster)" NAME="Debian GNU/Linux" VERSION_ID="10" VERSION="10 (buster)" VERSION_CODENAME=buster ID=debian HOME_URL="https://www.debian.org/" SUPPORT_URL="https://www.debian.org/support" BUG_REPORT_URL="https://bugs.debian.org/"

HighCommander4 commented 9 months ago

Please attach logs as instructed in the "Logs" section of the issue template.

coyorkdow commented 9 months ago

clangd.log

@HighCommander4 The log is very big since it's not a small project.

HighCommander4 commented 9 months ago

Thanks. I looked at the logs and don't see an obvious problem yet.

The symptoms sound like maybe the code contains a line like #define thread_local __thread, which would cause this error.

And I notice in the logs that the codebase contains a file named bsproxy/thread_local.h. Perhaps that file contains a #define like that?

coyorkdow commented 9 months ago

@HighCommander4 "bsproxy/thread_local.h" doesn't have such macro definition. But I was not sure if any dependency lib codes made such macro, so I did a simple verification.

#ifdef thread_local
#warning "you have defined a macro of \"thread_local\"!"
#undef thread_local
#endif

I add the above contents in my src file. And clangd gives me the warning. image However, this is a false positive because I can compile my project without the warning (actually I enabled -Werror). If I add a #define thread_local __thread manually before the verification, then the compiler will complain the errors.

So I believe my issue is resulted by that cland wrongly recognise the macro.

HighCommander4 commented 9 months ago

Go-to-definition on an occurrence of thread_local should take you to the macro definition that clangd is picking up.

coyorkdow commented 8 months ago

@HighCommander4 Sorry for late reply. I have found the macro definition (which cland thought it is).

#if !defined(thread_local) &&                                           \
    (__cplusplus < 201103L ||                                           \
     (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) < 40800)
// GCC supports thread_local keyword of C++11 since 4.8.0
#ifdef _MSC_VER
// WARNING: don't use this macro in C++03
#define thread_local __declspec(thread)
#else
// WARNING: don't use this macro in C++03
#define thread_local __thread
#endif  // _MSC_VER
#endif

So, as you can see. It uses some builtin macro to determine the cpp standard and compiler version. What I used is gcc6.3 and c++14. However, cland has a wrong recognition as the following screenshot shows: image

The correct values of __GNUC__, __GNUC_MINOR__, and __GNUC_PATCHLEVEL__ are 6, 3, 0, respectively. However, cland think they are 4, 2, 1.

coyorkdow commented 8 months ago

clangd.log There is another log, with respect to the preceding macro test. I can find some related stuff like

V[13:56:47.469] >>> {
  "id": 3509,
  "jsonrpc": "2.0",
  "result": {
    "contents": {
      "kind": "markdown",
      "value": "### macro `__GNUC__`  \n\n---\n```cpp\n#define __GNUC__ 4\n```"
    },
    "range": {
      "end": {
        "character": 14,
        "line": 29
      },
      "start": {
        "character": 6,
        "line": 29
      }
    }
  }
}

Hope it's helpful to locate the issue.

HighCommander4 commented 8 months ago

I have found the macro definition (which cland thought it is).

#if !defined(thread_local) &&                                           \
    (__cplusplus < 201103L ||                                           \
     (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) < 40800)
// GCC supports thread_local keyword of C++11 since 4.8.0
#ifdef _MSC_VER
// WARNING: don't use this macro in C++03
#define thread_local __declspec(thread)
#else
// WARNING: don't use this macro in C++03
#define thread_local __thread
#endif  // _MSC_VER
#endif

What file is this code in?

coyorkdow commented 8 months ago

@HighCommander4 It come from a thirdparty lib, and it is indirect including. The relative path is "***/***/***/***/thirdparty/brpc/src/butil/thread_local.h".

Since it's an open source lib, you can even find it on github. https://github.com/apache/brpc/blob/master/src/butil/thread_local.h

But what I use is not the newest version. These codes might be deleted from the brpc many years ago.

HighCommander4 commented 8 months ago

My understanding is that clang purposely does not try to pretend to be a specific (customizable) version of GCC, because there are going to be small differences between GCC versions and clang can't aim to be exactly compatible with all of them.

So, while you might build your code with gcc 6.3, clangd will not try to parse your code "as gcc 6.3", it will parse your code "as clang".

For historical reasons, all clang versions define the __GNUC__ macros __GNUC_MINOR__ macros to reflect version 4.2. (You can read more about that at https://stackoverflow.com/questions/12893731/why-does-clang-dumpversion-report-4-2-1 and the thread linked from that answer.)

When a library does feature detection, if the library intends to support clang, it should have a preprocessor branch where it checks the __clang__ macro and makes a decision based on the clang version.

Since this brpc library does not have such a branch here, I would characterize it as "this library code is not portable between gcc and clang".

Since clangd is based on clang, when trying to process code not portable to clang you're likely to run into issues (like this one).

As a workaround, you could try to use the clangd config file to override the values of __GNUC__ and __GNUC_MINOR__ to reflect version 6.3, for example with:

CompilerFlags:
  Add: [-D__GNUC__=6, -D__GNUC_MINOR__=3]

I'm not sure whether that might introduce other problems though.

HighCommander4 commented 8 months ago

When a library does feature detection, if the library intends to support clang, it should have a preprocessor branch where it checks the __clang__ macro and makes a decision based on the clang version.

Note additionally that these days there are better ways for libraries to detect support for language features, such as __has_feature(cxx_thread_local), which avoids the need for checking for individual compiler versions altogether.

coyorkdow commented 8 months ago

My understanding is that clang purposely does not try to pretend to be a specific (customizable) version of GCC, because there are going to be small differences between GCC versions and clang can't aim to be exactly compatible with all of them.

So, while you might build your code with gcc 6.3, clangd will not try to parse your code "as gcc 6.3", it will parse your code "as clang".

For historical reasons, all clang versions define the __GNUC__ macros __GNUC_MINOR__ macros to reflect version 4.2. (You can read more about that at https://stackoverflow.com/questions/12893731/why-does-clang-dumpversion-report-4-2-1 and the thread linked from that answer.)

When a library does feature detection, if the library intends to support clang, it should have a preprocessor branch where it checks the __clang__ macro and makes a decision based on the clang version.

Since this brpc library does not have such a branch here, I would characterize it as "this library code is not portable between gcc and clang".

Since clangd is based on clang, when trying to process code not portable to clang you're likely to run into issues (like this one).

As a workaround, you could try to use the clangd config file to override the values of __GNUC__ and __GNUC_MINOR__ to reflect version 6.3, for example with:

CompilerFlags:
  Add: [-D__GNUC__=6, -D__GNUC_MINOR__=3]

I'm not sure whether that might introduce other problems though.

Thank you for your explaining, I will get touch with the internal dependency providers and ask them if they can update this file so that make it portable between clang and gcc.