[PATCH] D54391: Fix compatibility with z3-4.8.1

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil created this revision.
jankratochvil added a reviewer: clang.
jankratochvil added a project: clang.

With `z3-4.8.1` as found in Fedora Rawhide (future Fedora 30) as 
`z3-devel-4.8.1-1.fc30.x86_64`:

  ../tools/clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp: In function 
'void {anonymous}::Z3ErrorHandler(Z3_context, Z3_error_code)':
  ../tools/clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:49:40: error: 
'Z3_get_error_msg_ex' was not declared in this scope
  llvm::Twine(Z3_get_error_msg_ex(Context, Error)));
  ^~~
  ../tools/clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:49:40: note: 
suggested alternative: 'Z3_get_error_msg'
  llvm::Twine(Z3_get_error_msg_ex(Context, Error)));
  ^~~
  Z3_get_error_msg

clang CMakeLists.txt has:

  find_package(Z3 4.7.1 EXACT)

Despite cmake documentation 

 claims "//If no such version file is available then the configuration file is 
assumed to not be compatible with any requested version.//" in reality 
`cmake-3.12.2-1.fc30.x86_64` with `z3-devel-4.8.1-1.fc30.x86_64` still does 
find it:

  -- Found Z3: /usr/lib64/libz3.so (Required is exact version "4.7.1")

clang is using `Z3_get_error_msg_ex()`, already `/usr/include/z3/z3_api.h` of 
`z3-devel-4.7.1-1.fc28.x86_64` states for it "`Retained function name for 
backwards compatibility within v4.1`" and it is implemented only as:

  Z3_API char const * Z3_get_error_msg_ex(Z3_context c, Z3_error_code err) {
  return Z3_get_error_msg(c, err);
  }


Repository:
  rC Clang

https://reviews.llvm.org/D54391

Files:
  lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp


Index: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -46,7 +46,7 @@
 // Function used to report errors
 void Z3ErrorHandler(Z3_context Context, Z3_error_code Error) {
   llvm::report_fatal_error("Z3 error: " +
-   llvm::Twine(Z3_get_error_msg_ex(Context, Error)));
+   llvm::Twine(Z3_get_error_msg(Context, Error)));
 }
 
 /// Wrapper for Z3 context


Index: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -46,7 +46,7 @@
 // Function used to report errors
 void Z3ErrorHandler(Z3_context Context, Z3_error_code Error) {
   llvm::report_fatal_error("Z3 error: " +
-   llvm::Twine(Z3_get_error_msg_ex(Context, Error)));
+   llvm::Twine(Z3_get_error_msg(Context, Error)));
 }
 
 /// Wrapper for Z3 context
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54391: Fix compatibility with z3-4.8.1

[Jan Kratochvil via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346635: Fix compatibility with z3-4.8.1 (authored by 
jankratochvil, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54391?vs=173555&id=173618#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54391

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp


Index: cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -46,7 +46,7 @@
 // Function used to report errors
 void Z3ErrorHandler(Z3_context Context, Z3_error_code Error) {
   llvm::report_fatal_error("Z3 error: " +
-   llvm::Twine(Z3_get_error_msg_ex(Context, Error)));
+   llvm::Twine(Z3_get_error_msg(Context, Error)));
 }
 
 /// Wrapper for Z3 context


Index: cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -46,7 +46,7 @@
 // Function used to report errors
 void Z3ErrorHandler(Z3_context Context, Z3_error_code Error) {
   llvm::report_fatal_error("Z3 error: " +
-   llvm::Twine(Z3_get_error_msg_ex(Context, Error)));
+   llvm::Twine(Z3_get_error_msg(Context, Error)));
 }
 
 /// Wrapper for Z3 context
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54535: cmake: z3: Remove EXACT from 4.7.1 after being compatible with 4.8.1

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil created this revision.
jankratochvil added a reviewer: mikhail.ramalho.
jankratochvil added a project: clang.
Herald added subscribers: cfe-commits, mgorny.

D54391 comment by @mikhail.ramalho  
says:

> Since we're supporting version 4.8.1 now, the cmake file should be changed to 
> "minimum" instead of "exact".


Repository:
  rC Clang

https://reviews.llvm.org/D54535

Files:
  CMakeLists.txt


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -410,7 +410,7 @@
 
 set(CLANG_ANALYZER_Z3_INSTALL_DIR "" CACHE STRING "Install directory of the Z3 
solver.")
 
-find_package(Z3 4.7.1 EXACT)
+find_package(Z3 4.7.1)
 
 if (CLANG_ANALYZER_Z3_INSTALL_DIR)
   if (NOT Z3_FOUND)


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -410,7 +410,7 @@
 
 set(CLANG_ANALYZER_Z3_INSTALL_DIR "" CACHE STRING "Install directory of the Z3 solver.")
 
-find_package(Z3 4.7.1 EXACT)
+find_package(Z3 4.7.1)
 
 if (CLANG_ANALYZER_Z3_INSTALL_DIR)
   if (NOT Z3_FOUND)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54535: cmake: z3: Remove EXACT from 4.7.1 after being compatible with 4.8.1

[Jan Kratochvil via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC347152: cmake: z3: Remove EXACT from 4.7.1 after being 
compatible with 4.8.1 (authored by jankratochvil, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D54535

Files:
  CMakeLists.txt


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -410,7 +410,7 @@
 
 set(CLANG_ANALYZER_Z3_INSTALL_DIR "" CACHE STRING "Install directory of the Z3 
solver.")
 
-find_package(Z3 4.7.1 EXACT)
+find_package(Z3 4.7.1)
 
 if (CLANG_ANALYZER_Z3_INSTALL_DIR)
   if (NOT Z3_FOUND)


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -410,7 +410,7 @@
 
 set(CLANG_ANALYZER_Z3_INSTALL_DIR "" CACHE STRING "Install directory of the Z3 solver.")
 
-find_package(Z3 4.7.1 EXACT)
+find_package(Z3 4.7.1)
 
 if (CLANG_ANALYZER_Z3_INSTALL_DIR)
   if (NOT Z3_FOUND)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 238260.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
@@ -0,0 +1,38 @@
+// RUN: %check_clang_tidy %s bugprone-pointer-cast-widening %t
+
+// 
+typedef __UINT64_TYPE__ uint64_t;
+typedef __INTPTR_TYPE__ intptr_t;
+typedef __UINTPTR_TYPE__ uintptr_t;
+
+// Do not assume ull_ns::uintptr_t to be uintptr_t.
+namespace ull_ns {
+typedef unsigned long long uintptr_t;
+}
+// This is how std::uintptr_t is defined in :
+namespace cstdint_ns {
+using ::uintptr_t;
+}
+
+void test() {
+  void *p = 0;
+  uint64_t u64cstyle = (uint64_t)p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  // uint64_t u64implicit = p; // error: cannot initialize a variable of type 'uint64_t' (aka 'unsigned long') with an lvalue of type 'void *' [clang-diagnostic-error]
+  uint64_t u64reinterpret = reinterpret_cast(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  // uint64_t u64static = static_cast(p); // error: static_cast from 'void *' to 'uint64_t' (aka 'unsigned long') is not allowed
+  uintptr_t up = (uintptr_t)p;
+  auto cup = (cstdint_ns::uintptr_t)p;
+  auto wup = (ull_ns::uintptr_t)p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'ull_ns::uintptr_t' (aka 'unsigned long long') which may sign-extend [bugprone-pointer-cast-widening]
+  typedef uintptr_t t1;
+  typedef t1 t2;
+  t2 t = (t2)p;
+  intptr_t ip = reinterpret_cast(p);
+  __uint128_t u64ipimplicit = ip;
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: do not use cast of pointer-like 'intptr_t' (aka 'long') to a wider type '__uint128_t' (aka 'unsigned __int128') which will sign-extend [bugprone-pointer-cast-widening]
+  __uint128_t u64ipstatic = static_cast<__uint128_t>(ip);
+  // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: do not use cast of pointer-like 'intptr_t' (aka 'long') to a wider type '__uint128_t' (aka 'unsigned __int128') which will sign-extend [bugprone-pointer-cast-widening]
+  // __uint128_t u64ipreinterpret = reinterpret_cast<__uint128_t>(ip); // error: reinterpret_cast from 'intptr_t' (aka 'long') to '__uint128_t' (aka 'unsigned __int128') is not allowed
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -70,6 +70,7 @@
`bugprone-multiple-statement-macro `_,
`bugprone-not-null-terminated-result `_, "Yes"
`bugprone-parent-virtual-call `_, "Yes"
+   `bugprone-pointer-cast-widening `_, "Yes"
`bugprone-posix-return `_, "Yes"
`bugprone-signed-char-misuse `_,
`bugprone-sizeof-container `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - bugprone-pointer-cast-widening
+
+bugprone-pointer-cast-widening
+==
+
+Checks for cast of a pointer to wider (even unsigned) integer as it
+sign-extends the pointer which happens on 32-bit hosts for 64-bit integers.
+The buggy behavior are 32-bit addresses printed like ``0xd56b5d60``.
+
+Casting from 32-bit ``void *`` to ``uint64_t`` requires an intermediate cast to
+``uintptr_t`` otherwise the pointer gets sign-extended:
+
+.. code-block:: c++
+
+  void *p=(void *)0x8000;
+  printf(  "%p""\n",p );//0x8000
+  printf("0x%" PRIxPTR "\n",(uintptr_t) p );//0x8000
+  printf("0x%" PRIxPTR "\n",reinterpret_cast(p));//0x8000
+  //^^^ recommended
+  printf("0x%" PRIx64  "\n",reinterpret_cast(p));//0x8000
+  printf("

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil marked 8 inline comments as done.
jankratochvil added a comment.

In D71707#1796671 , @aaron.ballman 
wrote:

> I agree that restricting casts to `intptr_t` is too restrictive.


Permitted `intptr_t`. But then implemented also checking a cast of `intptr_t` 
to wider integral types.

> Have you considered language extensions like `__ptr32`, `__ptr64`, `__sptr`, 
> and `__uptr` 
> (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we 
> also support?

I think only `__uptr` support would matter to prevent some false positives. I 
am not sure it is used anywhere. Should I really implement it?




Comment at: 
clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:21-22
+void PointerCastWideningCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus && !getLangOpts().C99)
+return;
+

aaron.ballman wrote:
> Why limiting this to C++ and >= C99? You can get pointer widening casts 
> through extensions in C89, for instance.
Removed the check completely.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp:36
+  for (QualType DestTypeCheck = DestType;;) {
+if (DestTypeCheck.getAsString() == "uintptr_t")
+  return;

aaron.ballman wrote:
> How well does this work with something like `std::uinptr_t` from ``?
It works fine because its
```
using::uintptr_t;
```
is recogized as a `typedef` in the loop above from namespace-less `uintptr_t'. 
Sure thanks for notifying me to check that.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp:17
+  t2 t = (t2)p;
+}

aaron.ballman wrote:
> I'd also like to see tests for implicit casts as well as pointer width and 
> sign extension language extensions.
Done implicit cases.  But I do not understand what you mean by the rest - could 
you provide an example? Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 347315.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/Shell/Expr/no_unique_address.cpp
  lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
@@ -44,18 +44,18 @@
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
 // CHECK: | |-FieldDecl {{.*}} A 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 5
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 5
 // CHECK: | |-FieldDecl {{.*}} B 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 7
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 7
 // CHECK: | |-FieldDecl {{.*}} C 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | |-FieldDecl {{.*}} D 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 15
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 15
 // CHECK: | |-FieldDecl {{.*}} E 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 1
 // CHECK: | |-FieldDecl {{.*}} F 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 2
 // CHECK: | |-FieldDecl {{.*}} G 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | `-FieldDecl {{.*}} H 'char'
-// CHECK: |   `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: |   {{.}}-IntegerLiteral {{.*}} 'int' 3
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7196,6 +7196,7 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6357,6 +6357,92 @@
 ToD->getTemplateSpecializationKind());
 }
 
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  auto retval = new ASTImporter(ToContext, ToFileManager, FromContext,
+FromFileManager, true /*MinimalImport*/,
+// We use the regular lookup.
+/*SharedState=*/nullptr);
+  retval->setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  return retval;
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+  R"(
+// lldb/test/API/commands/expression/codegen-crash-typedefdecl-not-in_declcontext/main.cpp
+template  struct DWrapper {};
+
+struct D {};
+
+namespace NS {
+typedef DWrapper DW;
+}
+
+struct B {
+  NS::DW spd;
+  int a = 0;
+};
+
+struct E {
+  E(B &b) : b_ref(b) {}
+  NS::DW f() { return {}; };
+  void g() {
+return; //%self.expect("p b_ref", substrs=['(B) $0 =', '(spd = NS::DW', 'a = 0)'])
+  }
+
+  B &b_ref;
+};
+B b;
+E e(b);
+  )",
+  Lang_CXX11);
+
+  auto *FromB = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+
+#if 0
+  // Set up a stub external storage.
+  FromTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  FromTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  Fro

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3684
+  return ToAttrOrErr.takeError();
+#if 0
+RecordType *FromRT =

It is still not working. The testcase either PASSes despite this `#if 0` or it 
FAILs even if it is `#if 1`. Sure the goal is to reproduce the LLDB testcase 
behavior - PASS with `#if 1` and FAIL with `#if 0`.
With libstdc++11 and this form of patch it fails on:
```
  lldb-api :: 
commands/expression/codegen-crash-typedefdecl-not-in_declcontext/TestCodegenCrashTypedefDeclNotInDeclContext.py
  lldb-api :: 
commands/expression/completion-crash-incomplete-record/TestCompletionCrashIncompleteRecord.py
  lldb-api :: 
commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
```
It also fails for this one but there are reasons not interesting for this patch:
```
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
```
These two are too complex to debug IMO:
```
  lldb-api :: 
commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
```
I tried to reproduce the first two, this one has [[ 
https://people.redhat.com/jkratoch/lldbbt1.txt | such backtrace ]]:
```
  lldb-api :: 
commands/expression/codegen-crash-typedefdecl-not-in_declcontext/TestCodegenCrashTypedefDeclNotInDeclContext.py
```



Comment at: clang/unittests/AST/ASTImporterTest.cpp:6441
+  findFromTU(FromB)->Importer->Imported(FromB, ToB);
+  llvm::Error Err = findFromTU(FromB)->Importer->ImportDefinition(FromB);
+  EXPECT_FALSE((bool)Err);

This code is trying to mimic the LLDB behavior from the backtrace of LLDB 
testcase: https://people.redhat.com/jkratoch/lldbbt1.txt



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 347475.
jankratochvil added a comment.

Update of the testcase but it still does not reproduce the LLDB problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/Shell/Expr/no_unique_address.cpp
  lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
@@ -44,18 +44,18 @@
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
 // CHECK: | |-FieldDecl {{.*}} A 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 5
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 5
 // CHECK: | |-FieldDecl {{.*}} B 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 7
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 7
 // CHECK: | |-FieldDecl {{.*}} C 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | |-FieldDecl {{.*}} D 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 15
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 15
 // CHECK: | |-FieldDecl {{.*}} E 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 1
 // CHECK: | |-FieldDecl {{.*}} F 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 2
 // CHECK: | |-FieldDecl {{.*}} G 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | `-FieldDecl {{.*}} H 'char'
-// CHECK: |   `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: |   {{.}}-IntegerLiteral {{.*}} 'int' 3
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7196,6 +7196,7 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6357,6 +6357,78 @@
 ToD->getTemplateSpecializationKind());
 }
 
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  auto retval = new ASTImporter(ToContext, ToFileManager, FromContext,
+FromFileManager, true /*MinimalImport*/,
+// We use the regular lookup.
+/*SharedState=*/nullptr);
+  retval->setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  return retval;
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+  R"(
+// lldb/test/API/commands/expression/codegen-crash-typedefdecl-not-in_declcontext/main.cpp
+template  struct DWrapper {};
+typedef DWrapper DW;
+struct B {
+  DW spd;
+} b;
+struct E {
+  B &b_ref = b;
+  static DW f() { return {}; }
+} e;
+  )",
+  Lang_CXX11);
+
+  auto *FromE = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("E")));
+
+#if 0
+  // Set up a stub external storage.
+  FromTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  FromTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  FromTU->getASTContext().setExternalSource(new TestExternalASTSource());
+#endif
+
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  R"(
+struct E;
+  )",
+  

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil added a comment.

  cat >1.cpp < struct DWrapper {};
  typedef DWrapper DW;
  struct B {
DW spd;
  } b;
  struct E {
B &b_ref = b;
static DW f() { return {}; }
  } e;
  EOH
  $ (set -ex;./bin/clang -c -o 1.o 1.cpp -Wall -g;./bin/lldb -b ./1.o -o 'p E' 
-o 'p e')
  (lldb) p E
  (lldb) p e
  lldb: .../clang/lib/AST/Decl.cpp:4188: bool 
clang::FieldDecl::isZeroSize(const clang::ASTContext &) const: Assertion 
`isInvalidDecl() && "valid field has incomplete type"' failed.

So importing just the type `E` is insufficient but I somehow cannot import the 
variable `e`:

auto *FromE = FirstDeclMatcher().match(FromTU, 
varDecl(hasName("e")));
  ...
// ASTTests: .../llvm/include/llvm/Support/Casting.h:269: typename 
cast_retty::ret_type llvm::cast(Y *) [X = clang::DeclContext, Y = 
clang::Decl]: Assertion `isa(Val) && "cast() argument of incompatible 
type!"' failed.
llvm::Error Err = findFromTU(FromE)->Importer->ImportDefinition(FromE);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 347478.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/Shell/Expr/no_unique_address.cpp
  lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
@@ -44,18 +44,18 @@
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
 // CHECK: | |-FieldDecl {{.*}} A 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 5
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 5
 // CHECK: | |-FieldDecl {{.*}} B 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 7
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 7
 // CHECK: | |-FieldDecl {{.*}} C 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | |-FieldDecl {{.*}} D 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 15
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 15
 // CHECK: | |-FieldDecl {{.*}} E 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 1
 // CHECK: | |-FieldDecl {{.*}} F 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 2
 // CHECK: | |-FieldDecl {{.*}} G 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | `-FieldDecl {{.*}} H 'char'
-// CHECK: |   `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: |   {{.}}-IntegerLiteral {{.*}} 'int' 3
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7196,6 +7196,7 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6357,6 +6357,78 @@
 ToD->getTemplateSpecializationKind());
 }
 
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  auto retval = new ASTImporter(ToContext, ToFileManager, FromContext,
+FromFileManager, true /*MinimalImport*/,
+// We use the regular lookup.
+/*SharedState=*/nullptr);
+  retval->setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  return retval;
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+  R"(
+// lldb/test/API/commands/expression/codegen-crash-typedefdecl-not-in_declcontext/main.cpp
+template  struct DWrapper {};
+typedef DWrapper DW;
+struct B {
+  DW spd;
+} b;
+struct E {
+  B &b_ref = b;
+  static DW f() { return {}; }
+} e;
+  )",
+  Lang_CXX11);
+
+  auto *FromE = FirstDeclMatcher().match(FromTU, varDecl(hasName("e")));
+
+#if 0
+  // Set up a stub external storage.
+  FromTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  FromTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  FromTU->getASTContext().setExternalSource(new TestExternalASTSource());
+#endif
+
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  R"(
+struct E;
+extern E e;
+  )",
+  Lang_CXX11);
+  auto *ToE = FirstDeclMatcher().match(ToTU, varDecl(hasName("e")));
+
+#if 1
+  // Set up a 

[PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 350361.
jankratochvil added a comment.
Herald added subscribers: llvm-commits, cfe-commits, dexonsmith, sstefan1, 
martong, hiraditya.
Herald added a reviewer: shafik.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: shafik.
Herald added projects: clang, LLVM.

`DW_AT_byte_size 0` implementation as suggested by @teemperor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101237/new/

https://reviews.llvm.org/D101237

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/test/Shell/Expr/no_unique_address.cpp
  llvm/include/llvm/IR/DebugInfoFlags.def
  llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

Index: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
@@ -1603,12 +1603,16 @@
   } else {
 addUInt(MemberDie, dwarf::DW_AT_data_bit_offset, None, Offset);
   }
+  assert(!(DT->getFlags() & DINode::FlagIsZeroSize) &&
+ "bitfields cannot have [[no_unique_address]]");
 } else {
   // This is not a bitfield.
   OffsetInBytes = DT->getOffsetInBits() / 8;
   if (AlignInBytes)
 addUInt(MemberDie, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,
 AlignInBytes);
+  if (DT->getFlags() & DINode::FlagIsZeroSize)
+addUInt(MemberDie, dwarf::DW_AT_byte_size, None, 0);
 }
 
 if (DD->getDwarfVersion() <= 2) {
Index: llvm/include/llvm/IR/DebugInfoFlags.def
===
--- llvm/include/llvm/IR/DebugInfoFlags.def
+++ llvm/include/llvm/IR/DebugInfoFlags.def
@@ -58,6 +58,8 @@
 HANDLE_DI_FLAG((1 << 27), BigEndian)
 HANDLE_DI_FLAG((1 << 28), LittleEndian)
 HANDLE_DI_FLAG((1 << 29), AllCallsDescribed)
+// FIXME: Move it to ReservedBit4?
+HANDLE_DI_FLAG((1 << 30), IsZeroSize)
 
 // To avoid needing a dedicated value for IndirectVirtualBase, we use
 // the bitwise or of Virtual and FwdDecl, which does not otherwise
@@ -67,7 +69,7 @@
 #ifdef DI_FLAG_LARGEST_NEEDED
 // intended to be used with ADT/BitmaskEnum.h
 // NOTE: always must be equal to largest flag, check this when adding new flag
-HANDLE_DI_FLAG((1 << 29), Largest)
+HANDLE_DI_FLAG((1 << 30), Largest)
 #undef DI_FLAG_LARGEST_NEEDED
 #endif
 
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -843,11 +843,10 @@
CompilerType *child_type = nullptr);
 
   // Modifying RecordType
-  static clang::FieldDecl *AddFieldToRecordType(const CompilerType &type,
-llvm::StringRef name,
-const CompilerType &field_type,
-lldb::AccessType access,
-uint32_t bitfield_bit_size);
+  static clang::FieldDecl *
+  AddFieldToRecordType(const CompilerType &type, llvm::StringRef name,
+   const CompilerType &field_type, lldb::AccessType access,
+   uint32_t bitfield_bit_size, bool IsZeroSize = false);
 
   static void BuildIndirectFields(const CompilerType &type);
 
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7164,7 +7164,7 @@
 clang::FieldDecl *TypeSystemClang::AddFieldToRecordType(
 const CompilerType &type, llvm::StringRef name,
 const CompilerType &field_clang_type, AccessType access,
-uint32_t bitfield_bit_size) {
+uint32_t bitfield_bit_size, bool IsZeroSize) {
   if (!type.IsValid() || !field_clang_type.IsValid())
 re

[PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil added inline comments.



Comment at: llvm/include/llvm/IR/DebugInfoFlags.def:62
+// FIXME: Move it to ReservedBit4?
+HANDLE_DI_FLAG((1 << 30), IsZeroSize)
 

This would need to be handled better if approved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101237/new/

https://reviews.llvm.org/D101237

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil added a comment.

[Dwarf-Discuss] How to map [[no_unique_address]] into DWARF: 
http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2021-June/004825.html


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101237/new/

https://reviews.llvm.org/D101237

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 350900.
jankratochvil edited the summary of this revision.
jankratochvil added a comment.

This patch now requires: D101236 , D103910 
 and D103966 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101237/new/

https://reviews.llvm.org/D101237

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/test/Shell/Expr/no_unique_address.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

Index: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
@@ -1603,12 +1603,16 @@
   } else {
 addUInt(MemberDie, dwarf::DW_AT_data_bit_offset, None, Offset);
   }
+  assert(!(DT->getFlags() & DINode::FlagIsZeroSize) &&
+ "bitfields cannot have [[no_unique_address]]");
 } else {
   // This is not a bitfield.
   OffsetInBytes = DT->getOffsetInBits() / 8;
   if (AlignInBytes)
 addUInt(MemberDie, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,
 AlignInBytes);
+  if (DT->getFlags() & DINode::FlagIsZeroSize)
+addUInt(MemberDie, dwarf::DW_AT_byte_size, None, 0);
 }
 
 if (DD->getDwarfVersion() <= 2) {
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -843,11 +843,10 @@
CompilerType *child_type = nullptr);
 
   // Modifying RecordType
-  static clang::FieldDecl *AddFieldToRecordType(const CompilerType &type,
-llvm::StringRef name,
-const CompilerType &field_type,
-lldb::AccessType access,
-uint32_t bitfield_bit_size);
+  static clang::FieldDecl *
+  AddFieldToRecordType(const CompilerType &type, llvm::StringRef name,
+   const CompilerType &field_type, lldb::AccessType access,
+   uint32_t bitfield_bit_size, bool IsZeroSize = false);
 
   static void BuildIndirectFields(const CompilerType &type);
 
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7164,7 +7164,7 @@
 clang::FieldDecl *TypeSystemClang::AddFieldToRecordType(
 const CompilerType &type, llvm::StringRef name,
 const CompilerType &field_clang_type, AccessType access,
-uint32_t bitfield_bit_size) {
+uint32_t bitfield_bit_size, bool IsZeroSize) {
   if (!type.IsValid() || !field_clang_type.IsValid())
 return nullptr;
   TypeSystemClang *ast =
@@ -7196,6 +7196,8 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+if (IsZeroSize)
+  field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1264,8 +1264,10 @@
   if (location_type == PDB_LocType::ThisRel)
 bit_size *= 8;
 
+  bool IsZeroSize = false; // FIXME: Is there [[no_unique_address]] in PDB?
   auto decl = TypeSystemClang::AddFieldToRecordType(
-  record_type, member_name.c_str(), member_comp_type, access, bit_size);
+  record_type, member_name.c_str()

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil abandoned this revision.
jankratochvil added a comment.

Thanks for the reminder, I have todolisted it now for myself but for now I am 
abandoning it as I cannot promise anything.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil created this revision.
jankratochvil added reviewers: labath, alexfh, lebedev.ri.
jankratochvil added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, kristof.beyls, mgorny.
Herald added a project: clang.

Check for cast of a pointer to wider (even unsigned) integer. This will 
sign-extend the pointer which happens on 32-bit hosts for 64-bit integers:

  void *p=(void *)0x8000;
  printf(  "%p""\n",p ); // 0x8000
  printf("0x%" PRIxPTR "\n", (uintptr_t)p ); // 0x8000
  printf("0x%" PRIx64  "\n",
 reinterpret_cast(p)); // 0x8000
  printf("0x%" PRIx64  "\n", (uint64_t) p ); // 0x8000
  printf("0x%" PRIx64  "\n",   (uint64_t)(uintptr_t)p ); // 0x8000

It has caught a bug on ARM32: D71498  (and 
D71514 )


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71707

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s bugprone-pointer-cast-widening %t
+
+#include 
+
+void test(void) {
+  void *p = 0;
+  uint64_t u64 = (uint64_t)p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  uint64_t u64r = reinterpret_cast(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  intptr_t ip = reinterpret_cast(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
+  uintptr_t up = (uintptr_t)p;
+  typedef uintptr_t t1;
+  typedef t1 t2;
+  t2 t = (t2)p;
+}
Index: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
@@ -0,0 +1,42 @@
+//===--- PointerCastWideningCheck.h - clang-tidy-*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POINTER_CAST_WIDENING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POINTER_CAST_WIDENING_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Check for cast of a pointer to wider (even unsigned) integer. This will
+/// sign-extend the pointer which happens on 32-bit hosts for 64-bit integers.
+///
+/// \code
+/// void *p=(void *)0x8000;
+/// printf(  "%p""\n",p ); // 0x8000
+/// printf("0x%" PRIxPTR "\n", (uintptr_t)p ); // 0x8000
+/// printf("0x%" PRIx64  "\n",
+///reinterpret_cast(p)); // 0x8000
+/// printf("0x%" PRIx64  "\n", (uint64_t) p ); // 0x8000
+/// printf("0x%" PRIx64  "\n",   (uint64_t)(uintptr_t)p ); // 0x8000
+/// \endcode
+class PointerCastWideningCheck : public ClangTidyCheck {
+public:
+  PointerCastWideningCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POINTER_CAST_WIDENING_H
Index: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
@@ -0,0 +1,54 @@
+//===--- PointerCastWideningCheck.cpp - clang-tidy-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil added a comment.

Example output:

  PATH="$PWD/bin:$PATH" 
~/redhat/llvm-monorepo/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py 
-checks='-*,bugprone-pointer-cast-widening' 
/home/jkratoch/redhat/llvm-monorepo/lldb/source/
  
  Enabled checks:
  bugprone-pointer-cast-widening
  
  /home/jkratoch/redhat/llvm-monorepo/lldb/source/Core/PluginManager.cpp:89:35: 
warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 
'long') which may sign-extend [bugprone-pointer-cast-widening]
return reinterpret_cast(reinterpret_cast(VPtr));
^
   - What is the use of intptr_t?
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:333:29:
 warning: do not use cast of a pointer 
'std::__shared_ptr::element_type 
*' (aka 'lldb_private::Process *') to non-uintptr_t 'lldb::addr_t' (aka 
'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
__FUNCTION__, (lldb::addr_t)process_sp.get(),
  ^
   - A bug, submitted as: D71514
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:582:42:
 warning: do not use cast of a pointer 'const uint8_t *' (aka 'const unsigned 
char *') to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may 
sign-extend [bugprone-pointer-cast-widening]
(uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
   ^
   - A bug, not yet submitted.
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:713:42:
 warning: do not use cast of a pointer 'uint8_t *' (aka 'unsigned char *') to 
non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend 
[bugprone-pointer-cast-widening]
(uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
   ^
   - A bug, not yet submitted.
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/common/HostInfoBase.cpp:252:32:
 warning: do not use cast of a pointer 'bool (*)(lldb_private::FileSpec &)' to 
non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend 
[bugprone-pointer-cast-widening]
reinterpret_cast(reinterpret_cast(
 ^
   - What is the use of intptr_t?
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/posix/DomainSocket.cpp:48:20:
 warning: do not use cast of a pointer 'char *' to non-uintptr_t 'size_t' (aka 
'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
  saddr_un_len = SUN_LEN(saddr_un);
 ^
  /usr/include/sys/un.h:40:24: note: expanded from macro 'SUN_LEN'
  # define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)
\
 ^
   - False positive?
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRExecutionUnit.cpp:351:53:
 warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' 
(aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
  function.getName().str().c_str(), external, (lldb::addr_t)fun_ptr));
  ^
   - A bug, submitted as: D71498
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp:358:24:
 warning: do not use cast of a pointer 'clang::NamedDecl *' to non-uintptr_t 
'uint64_t' (aka 'unsigned long') which may sign-extend 
[bugprone-pointer-cast-widening]
 reinterpret_cast(result_decl), false);
 ^
   - A bug, not yet submitted.
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:139:23:
 warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' 
(aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
  AppendBounds(str, reinterpret_cast(info.si_lower),
^
   - A bug, submitted as: D71514
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:140:18:
 warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' 
(aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
   reinterpret_cast(info.si_upper),
   ^
   - A bug, submitted as: D71514
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:141:18:
 warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' 
(aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
   reinterpret_cast(info.si_addr));
   ^
   - A bug, submitted as: D71514
  
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:147:31:
 warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' 
(aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil added a subscriber: labath.
jankratochvil added a comment.

In D71707#1791280 , @labath wrote:

> - disallowing casts to intptr_t seems too restrictive -- I doubt many people 
> are doing that, but I guess this type exists for a reason, and since the type 
> (and it's signedness) is spelled out in the source, it shouldn't be too 
> surprising that sign-extension can happen later


I was trying to find what is `intptr_t` good for and I haven't found any valid 
reason. It seems to me nobody knows that either. Which is why I find correct to 
report it. This checker has many false positives (or "not really a bug") anyway.

> - requiring a literal uintptr_t (or a typedef of it) may be also problematic 
> -- the user could obtain an integer type of the same bit width through some 
> other means (e.g. `#ifdef`). OTOH, without that (and just checking the bit 
> width for instance), one would have to actually compile for a 32-bit target 
> to get this warning. I don't know what's the practice for this in 
> clang-tidy...

Yes, I wanted first to check the widths but then I realized user would need a 
32-bit host for that which is too difficult to (1) get nowadays and (2) 
primarily to build there LLVM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 234772.
jankratochvil added a comment.

Wrote the documentation and Release Notes entry, thanks for the review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s bugprone-pointer-cast-widening %t
+
+#include 
+
+void test(void) {
+  void *p = 0;
+  uint64_t u64 = (uint64_t)p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  uint64_t u64r = reinterpret_cast(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  intptr_t ip = reinterpret_cast(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
+  uintptr_t up = (uintptr_t)p;
+  typedef uintptr_t t1;
+  typedef t1 t2;
+  t2 t = (t2)p;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -62,6 +62,7 @@
bugprone-multiple-statement-macro
bugprone-not-null-terminated-result
bugprone-parent-virtual-call
+   bugprone-pointer-cast-widening
bugprone-posix-return
bugprone-sizeof-container
bugprone-sizeof-expression
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - bugprone-pointer-cast-widening
+
+bugprone-pointer-cast-widening
+==
+
+Check for cast of a pointer to wider (even unsigned) integer. This will
+sign-extend the pointer which happens on 32-bit hosts for 64-bit integers.
+The buggy behavior are 32-bit addresses printed like ``0xd56b5d60``.
+
+Casting from 32-bit ``void *`` to ``uint64_t`` requires an intermediate cast to
+``uintptr_t`` otherwise the pointer gets sign-extended:
+
+.. code-block:: c++
+
+  void *p=(void *)0x8000;
+  printf(  "%p""\n",p );//0x8000
+  printf("0x%" PRIxPTR "\n",(uintptr_t) p );//0x8000
+  printf("0x%" PRIxPTR "\n",reinterpret_cast(p));//0x8000
+^^^ recommended
+  printf("0x%" PRIx64  "\n",reinterpret_cast(p));//0x8000
+  printf("0x%" PRIx64  "\n",(uint64_t ) p );//0x8000
+  printf("0x%" PRIx64  "\n",  (uint64_t)(uintptr_t) p );//0x8000
+
+The only supported cast from a pointer to integer is ``uintptr_t``.
+
+Casting to ``int64_t`` is discouraged as it leads to the sign-extensions again.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   Without the null terminator it can result in undefined behaviour when the
   string is read.
 
+- New :doc:`bugprone-pointer-cast-widening
+  ` check
+
+  Check for cast of a pointer to wider (even unsigned) integer. This will
+  sign-extend the pointer which happens on 32-bit hosts for 64-bit integers.
+
 - New :doc:`cert-mem57-cpp
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
@@ -0,0 +1,42 @@
+//===--- PointerCastWideningCheck.h - clang-tidy-*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WIT

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 234787.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s bugprone-pointer-cast-widening %t
+
+#include 
+
+void test() {
+  void *p = 0;
+  uint64_t u64 = (uint64_t)p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  uint64_t u64r = reinterpret_cast(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
+  intptr_t ip = reinterpret_cast(p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
+  uintptr_t up = (uintptr_t)p;
+  typedef uintptr_t t1;
+  typedef t1 t2;
+  t2 t = (t2)p;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -62,6 +62,7 @@
bugprone-multiple-statement-macro
bugprone-not-null-terminated-result
bugprone-parent-virtual-call
+   bugprone-pointer-cast-widening
bugprone-posix-return
bugprone-sizeof-container
bugprone-sizeof-expression
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - bugprone-pointer-cast-widening
+
+bugprone-pointer-cast-widening
+==
+
+Checks for cast of a pointer to wider (even unsigned) integer as it
+sign-extends the pointer which happens on 32-bit hosts for 64-bit integers.
+The buggy behavior are 32-bit addresses printed like ``0xd56b5d60``.
+
+Casting from 32-bit ``void *`` to ``uint64_t`` requires an intermediate cast to
+``uintptr_t`` otherwise the pointer gets sign-extended:
+
+.. code-block:: c++
+
+  void *p=(void *)0x8000;
+  printf(  "%p""\n",p );//0x8000
+  printf("0x%" PRIxPTR "\n",(uintptr_t) p );//0x8000
+  printf("0x%" PRIxPTR "\n",reinterpret_cast(p));//0x8000
+  //^^^ recommended
+  printf("0x%" PRIx64  "\n",reinterpret_cast(p));//0x8000
+  printf("0x%" PRIx64  "\n",(uint64_t ) p );//0x8000
+  printf("0x%" PRIx64  "\n",  (uint64_t)(uintptr_t) p );//0x8000
+
+The only supported cast from a pointer to integer is ``uintptr_t``.
+
+Casting to ``int64_t`` is discouraged as it leads to the sign-extensions again.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,12 @@
   Without the null terminator it can result in undefined behaviour when the
   string is read.
 
+- New :doc:`bugprone-pointer-cast-widening
+  ` check
+
+  Checks for cast of a pointer to wider (even unsigned) integer as it
+  sign-extends the pointer which happens on 32-bit hosts for 64-bit integers.
+
 - New :doc:`cert-mem57-cpp
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
@@ -0,0 +1,32 @@
+//===--- PointerCastWideningCheck.h - clang-tidy-*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef L

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil marked 3 inline comments as done.
jankratochvil added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:100
+
+  Check for cast of a pointer to wider (even unsigned) integer. This will
+  sign-extend the pointer which happens on 32-bit hosts for 64-bit integers.

Eugene.Zelenko wrote:
> Checks. One sentence should be enough.
Not sure if the wording is OK in such a long sentence (non-native English here).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71707/new/

https://reviews.llvm.org/D71707



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil created this revision.
jankratochvil added reviewers: teemperor, shafik, dblaikie.
jankratochvil added a project: clang.
Herald added a subscriber: martong.
Herald added a reviewer: a.sidorin.
jankratochvil requested review of this revision.

When LLDB needs to use `[[no_unique_address]]` (which is in fact always but 
that is topic of the next patch) `FieldDecl::isZeroSize` was missing the 
`NoUniqueAddressAttr` attribute calculating wrong field offsets and thus 
failing on an assertion:
lldb: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:802: void (anonymous 
namespace)::CGRecordLowering::insertPadding(): Assertion `Offset >= Size' 
failed.
After importing just the `NoUniqueAddressAttr` attribute various testcases were 
failing on:
clang/lib/AST/Decl.cpp:4163: bool clang::FieldDecl::isZeroSize(const 
clang::ASTContext &) const: Assertion `isInvalidDecl() && "valid field has 
incomplete type"' failed.
The LLDB testsuite exercises the patch fine. But the clang testcase I have 
provided here succeeds even with no patch applied. I expect I would need to 
import it X->Y->Z and not just X->Y but despite I tried some such code it was 
still working even without my patch.
Is it OK without a testcase or is there some suggestion how to write the clang 
testcase?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,38 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  auto SrcCode =
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )";
+  Decl *FromTU = getTuDecl(SrcCode, Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+  auto *BTo2D = Import(BToD, Lang_CXX20);
+  EXPECT_TRUE(BTo2D);
+
+  // It does not fail even without the patch!
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+  auto *CTo2D = Import(CToD, Lang_CXX20);
+  EXPECT_TRUE(CTo2D);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (auto FromAttr = D->getAttr()) {
+if (auto ToAttrOrErr = Importer.Import(FromAttr))
+  ToField->addAttr(*ToAttrOrErr);
+else
+  return ToAttrOrErr.takeError();
+RecordType *FromRT =
+const_cast(D->getType()->getAs());
+// Is this field a struct? FieldDecl::isZeroSize will need its definition.
+if (FromRT) {
+  RecordDecl *FromRD = FromRT->getDecl();
+  RecordType *ToRT =
+  const_cast(ToField->getType()->getAs());
+  assert(ToRT && "RecordType import has different type");
+  RecordDecl *ToRD = ToRT->getDecl();
+  if (FromRD->isCompleteDefinition() && !ToRD->isCompleteDefinition()) {
+if (Error Err = ImportDefinition(FromRD, ToRD, IDK_Basic))
+  return std::move(Err);
+  }
+}
+  }
+
   ToField->setAccess(D->getAccess());
   ToField->setLexicalDeclContext(LexicalDC);
   if (ToInitializer)
@@ -8445,6 +8467,8 @@
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
 
+  // There may have been NoUniqueAddressAttr for FieldDecl::isZeroSize.
+  ToD->dropAttrs();
   if (FromD->hasAttrs())
 for (const Attr *FromAttr : FromD->getAttrs()) {
   auto ToAttrOrErr = Import(FromAttr);


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,38 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  auto SrcCode =
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )";
+  Decl *FromTU = getTuDecl(SrcCode, Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD = Fi

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 340340.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,33 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+
+  // It does not fail even without the patch!
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (const Attr *FromAttr = D->getAttr()) {
+if (auto ToAttrOrErr = Importer.Import(FromAttr))
+  ToField->addAttr(*ToAttrOrErr);
+else
+  return ToAttrOrErr.takeError();
+RecordType *FromRT =
+const_cast(D->getType()->getAs());
+// Is this field a struct? FieldDecl::isZeroSize will need its definition.
+if (FromRT) {
+  RecordDecl *FromRD = FromRT->getDecl();
+  RecordType *ToRT =
+  const_cast(ToField->getType()->getAs());
+  assert(ToRT && "RecordType import has different type");
+  RecordDecl *ToRD = ToRT->getDecl();
+  if (FromRD->isCompleteDefinition() && !ToRD->isCompleteDefinition()) {
+if (Error Err = ImportDefinition(FromRD, ToRD, IDK_Basic))
+  return std::move(Err);
+  }
+}
+  }
+
   ToField->setAccess(D->getAccess());
   ToField->setLexicalDeclContext(LexicalDC);
   if (ToInitializer)
@@ -8445,6 +8467,8 @@
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
 
+  // There may have been NoUniqueAddressAttr for FieldDecl::isZeroSize.
+  ToD->dropAttrs();
   if (FromD->hasAttrs())
 for (const Attr *FromAttr : FromD->getAttrs()) {
   auto ToAttrOrErr = Import(FromAttr);


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,33 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+
+  // It does not fail even without the patch!
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (const Attr *FromAttr = D->getAttr()) {
+if (auto ToAttrOrErr = Importer.Import(FromAttr))
+  ToField->addAttr(*ToAttrOrErr);
+else
+  return ToAttrOrErr.takeError();
+RecordType *FromRT =
+const_cast(D->getType()->getAs());
+// Is this field a struct? FieldDecl::isZeroSize will need its definition.
+if (FromRT) {
+  RecordDecl *FromRD = FromRT->getDecl();
+  RecordType *ToRT =
+  const_cast(ToField->getType()->getAs());
+

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil planned changes to this revision.
jankratochvil added a comment.

In D101236#2716286 , @teemperor wrote:

> Not sure what the backtrace is for the `clang::FieldDecl::isZeroSize` crash 
> but what might relevant:

That happens only with half of the patch: 
https://people.redhat.com/jkratoch/lldb-iszerosize.patch
And only for these testcases:

  lldb-api :: 
commands/expression/codegen-crash-typedefdecl-not-in_declcontext/TestCodegenCrashTypedefDeclNotInDeclContext.py
  lldb-api :: 
commands/expression/completion-crash-incomplete-record/TestCompletionCrashIncompleteRecord.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py

The backtrace is: https://people.redhat.com/jkratoch/lldb-iszerosize.txt

> IMHO this is fine as a temporary solution until we finally fix the LLDB Decl 
> completion.

Great, thanks.

In D101236#2716317 , @martong wrote:

> You can create a similar descendant class, but with the MinimalImport flag 
> set to true. Then you could call `ImportDefinition` subsequently after an 
> `Import` call. Perhaps that could trigger your assertion.

I have been looking for such suggestion, thanks. I will try that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil updated this revision to Diff 342249.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,105 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+
+  QualType CFType = FromTU->getASTContext().getRecordType(CFromD);
+  TypeInfo TFI = FromTU->getASTContext().getTypeInfo(CFType);
+  ASSERT_EQ(TFI.Width, 8U);
+  QualType CTType = FromTU->getASTContext().getRecordType(CToD);
+  TypeInfo TTI = FromTU->getASTContext().getTypeInfo(CTType);
+  // Here we test the NoUniqueAddressAttr import.
+  // But the Record definition part is not tested here.
+  ASSERT_EQ(TTI.Width, TFI.Width);
+}
+
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, true /*MinimalImport*/,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+  auto *ToC = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("C")));
+
+  // Set up a stub external storage.
+  ToTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  ToTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  ToTU->getASTContext().setExternalSource(new TestExternalASTSource());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+struct C;
+  )",
+  Lang_CXX20);
+  auto *FromC = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  auto *ImportedC = Import(FromC, Lang_CXX20);
+  // The lookup must find the existing class definition in the LinkageSpecDecl.
+  // Then the importer renders the existing and the new decl into one chain.
+  EXPECT_EQ(ImportedC->getCanonicalDecl(), ToC->getCanonicalDecl());
+
+  // Import the definition of the created class.
+  llvm::Error Err = findFromTU(FromC)->Importer->ImportDefinition(ToC);
+  EXPECT_FALSE((bool)Err);
+  consumeError(std::move(Err));
+
+  QualType CTType = ToTU->getASTContext().getRecordType(ToC);
+  TypeInfo TTI = ToTU->getASTContext().getTypeInfo(CTType);
+  fprintf(stderr,"LLDB To Width=%lu\n",TTI.Width);
+#if 0 // getASTRecordLayout(const clang::RecordDecl *) const: Assertion `D && "Cannot get layout of forward declarations!"' failed.
+  QualType CFType = FromTU->getASTContext().getRecordType(FromC);
+  TypeInfo TFI = FromTU->getASTContext().getTypeInfo(CFType);
+  fprintf(stderr,"LLDB From Width=%lu\n",TFI.Width);
+#endif
+  QualType CIType = FromTU->getASTContext().getRecordType(ImportedC);
+  TypeInfo TII = FromTU->getASTContext().getTypeInfo(CIType);
+  fprintf(stderr,"LLDB Imported Width=%lu\n",TII.Width);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
@@ -6397,5 +6496,8 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportWithExternalSource,
 DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBMinimalImport,
+DefaultTestValuesForRunOptions, );
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,30 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  /

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil added a comment.

In D101236#2716317 , @martong wrote:

> You can create a similar descendant class, but with the MinimalImport flag 
> set to true. Then you could call `ImportDefinition` subsequently after an 
> `Import` call. Perhaps that could trigger your assertion.

Do you have another hint or should I try harder? Thanks.

  clang/lib/Basic/SourceManager.cpp:865: clang::FileID 
clang::SourceManager::getFileIDLoaded(unsigned int) const: Assertion `0 && 
"Invalid SLocOffset or bad function choice"' failed.
  * thread #1, name = 'ASTTests', stop reason = hit program assert
  frame #4: 0x028d7195 
ASTTests`clang::SourceManager::getFileIDLoaded(this=0x04ecf640, 
SLocOffset=14405) const at SourceManager.cpp:865:5
 862  FileID SourceManager::getFileIDLoaded(unsigned SLocOffset) const {
 863// Sanity checking, otherwise a bug may lead to hanging in release 
build.
 864if (SLocOffset < CurrentLoadedOffset) {
  -> 865  assert(0 && "Invalid SLocOffset or bad function choice");
 866  return FileID();
 867}
 868
  (lldb) p SLocOffset
  (unsigned int) $0 = 14405
  (lldb) p CurrentLoadedOffset
  (unsigned int) $1 = 2147483648
  (lldb) bt
  frame # 3: libc.so.6`__assert_fail + 70
* frame # 4: 
ASTTests`clang::SourceManager::getFileIDLoaded(this=0x04ecf640, 
SLocOffset=14405) const at SourceManager.cpp:865:5
  frame # 5: 
ASTTests`clang::SourceManager::getFileIDSlow(this=0x04ecf640, 
SLocOffset=14405) const at SourceManager.cpp:773:10
  frame # 6: 
ASTTests`clang::SourceManager::getFileID(clang::SourceLocation) const at 
SourceManager.h:1107:12
  frame # 7: 
ASTTests`clang::SourceManager::getDecomposedExpansionLoc(this=0x04ecf640,
 Loc=(ID = 14405)) const at SourceManager.h:1247:18
  frame # 8: 
ASTTests`clang::SourceManager::getPresumedLoc(this=0x04ecf640, Loc=(ID 
= 14405), UseLineDirectives=true) const at SourceManager.cpp:1521:41
  frame # 9: 
ASTTests`clang::SourceManager::isWrittenInBuiltinFile(this=0x04ecf640, 
Loc=(ID = 14405)) const at SourceManager.h:1468:24
  frame #10: ASTTests`clang::ASTImporter::Import(this=0x778f9010, 
FromLoc=(ID = 14405)) at ASTImporter.cpp:8834:27
  frame #11: ASTTests`llvm::Error 
clang::ASTImporter::importInto(this=0x778f9010, 
To=0x7fffbe68, From=0x7fffb8e8) at ASTImporter.h:336:22
  frame #12: ASTTests`llvm::Error 
clang::ASTNodeImporter::importInto(this=0x7fffbf60,
 To=0x7fffbe68, From=0x7fffb8e8) at ASTImporter.cpp:148:23
  frame #13: 
ASTTests`clang::ASTNodeImporter::ImportDeclParts(this=0x7fffbf60, 
D=0x04eb8190, DC=0x7fffbe80, LexicalDC=0x7fffbe78, 
Name=0x7fffbe70, ToD=0x7fffbe60, Loc=0x7fffbe68) at 
ASTImporter.cpp:1654:19
  frame #14: 
ASTTests`clang::ASTNodeImporter::VisitRecordDecl(this=0x7fffbf60, 
D=0x04eb8190) at ASTImporter.cpp:2772:19
  frame #15: ASTTests`clang::declvisitor::Base 
>::VisitCXXRecordDecl(this=0x7fffbf60, D=0x04eb8190) at 
DeclNodes.inc:263:1
  frame #16: ASTTests`clang::declvisitor::Base 
>::Visit(this=0x7fffbf60, D=0x04eb8190) at DeclNodes.inc:263:1
  frame #17: 
ASTTests`clang::ASTImporter::ImportImpl(this=0x778f9010, 
FromD=0x04eb8190) at ASTImporter.cpp:8216:19
  frame #18: ASTTests`clang::ASTImporter::Import(this=0x778f9010, 
FromD=0x04eb8190) at ASTImporter.cpp:8387:27
  frame #19: ASTTests`llvm::Expected 
clang::ASTNodeImporter::import(this=0x7fffc6c8, 
From=0x04eb8190) at ASTImporter.cpp:164:31
  frame #20: 
ASTTests`clang::ASTNodeImporter::ImportDeclContext(this=0x7fffc6c8, 
FromDC=0x04e886a8, ForceImport=true) at ASTImporter.cpp:1777:34
  frame #21: 
ASTTests`clang::ASTImporter::ImportDefinition(this=0x778f9010, 
From=0x04e88668) at ASTImporter.cpp:9072:19
  frame #22: 
ASTTests`clang::ast_matchers::LLDBMinimalImport_LLDBImportNoUniqueAddress_Test::TestBody()
 at ASTImporterTest.cpp:6408:50


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil abandoned this revision.
jankratochvil added a comment.

IIUC it will get replaced by D101950 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits