[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I've reverted this 
https://github.com/llvm/llvm-project/commit/ffc67bb3602a6a9a4f886af362e1f2d7c9821570
 as Linaro's flang and clang bots are red all over and we're also in an upgrade 
period for the build server 
(https://discourse.llvm.org/t/llvm-zorg-migration-to-the-buildbot-v3-9/73749) 
so getting a fix into zorg isn't going to happen for a few days at least.

Whoever decided to land this change please look at the remedies suggested by 
@pscoro and submit PRs to address those and eventually reland this. Happy to 
help on review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154869

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

[PATCH] D49549: Change 'clang-test' to 'check-clang' on the hacking webpage

[David Spickett via Phabricator via cfe-commits]

DavidSpickett closed this revision.
DavidSpickett added a comment.
Herald added a project: All.

I updated this to the monorepo layout and landed it: 
https://github.com/llvm/llvm-project/commit/604a231881a0e8f204a6d3337bfa3e797e911e34

Given the long time this has been here and the last activity of Arnaud here 
I've credited them by name in the commit message as was done for their other 
changes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D49549

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

[PATCH] D119008: Add Cortex-X1C to Clang LLVM 14 release notes

[David Spickett via Phabricator via cfe-commits]

DavidSpickett closed this revision.
DavidSpickett added a comment.
Herald added a project: All.

Landed on 14.x as 
https://github.com/llvm/llvm-project/commit/56dcb10a9942c206d53a59abd66c2207a01b39fa.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119008

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

[PATCH] D74031: Update for Clang 10 release notes in order to have reference to D66404.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett closed this revision.
DavidSpickett added a comment.
Herald added a project: All.

Was landed on 10.x as 
https://github.com/llvm/llvm-project/commit/b4efc29f1ccbc03453590bf7aae337853c91c91f.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74031

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

[PATCH] D154093: [clang-format] Break long string literals in C#, etc.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

This was causing failures on a large number of Linaro's bots. Judging by how 
widespread it was, reproducing could be as simple as adding 
`-DLLVM_ENABLE_ASSERTIONS=True` to your CMake config.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154093

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

[PATCH] D158307: [flang][driver] Disable Clang options in Flang

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

For reasons I won't get into, buildbots don't show what llvm-test-suite 
revision they are using but we can be sure by now that the other change has 
gone through. Fingers crossed for this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158307

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

[PATCH] D158289: [flang][driver] Partial revert of https://reviews.llvm.org/D157837

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158289

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

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Failing on one of our 2 stage AArch64 bots 
https://lab.llvm.org/buildbot/#/builders/176/builds/4267 after the reland (that 
bot has been red for various reasons today, so no emails got sent).

Maybe it helps to know that it doesn't fail on the same config with single 
stage https://lab.llvm.org/buildbot/#/builders/197/builds/8998. So it's at 
least able to build simple test cases (the test suite is failing to build for a 
separate reason).

If this is somehow AArch64 only we (Linaro) can help investigate, I see that it 
was ok on PowerPC for example 
https://lab.llvm.org/buildbot/#/builders/36/builds/36728.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156571

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

[PATCH] D158218: [CMake] Deprecate DEFAULT_SYSROOT and GCC_INSTALL_PREFIX

[David Spickett via Phabricator via cfe-commits]

DavidSpickett resigned from this revision.
DavidSpickett added a comment.

I do cross build lldb a lot but never needed to set either of these, so I have 
no opinion on this.

> clang/cmake/caches/CrossWinToARMLinux.cmake

Is not one of Linaro's, there's a buildbot for it 
(https://lab.llvm.org/buildbot/#/builders/60) owned by `Vladimir Vereschaka 
`.

Of course Linaro can help if porting it, if it proves difficult.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158218

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

[PATCH] D157808: [clang] Add missing field to VisibilityAttr json AST dump

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

We also had a failure looking for the mangled name on Windows on Arm: 
https://lab.llvm.org/buildbot/#/builders/65/builds/10954

The triple fix may take care of that too.

> Also, consider spreading out commits a bit so that if one breaks something, 
> it's easily to see which one it was.

Definitely.

Another option for dependent patches that must go in together is to add `[1/N]` 
so we know it's essentially one large change, and can revert accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157808

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

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

One of the kernel tests now fails to build for AArch64 and Arm:

  00:00:48 ++ make 
CC=/home/tcwg-buildslave/workspace/tcwg_kernel_0/bin/aarch64-cc 
LD=/home/tcwg-buildslave/workspace/tcwg_kernel_0/llvm-install/bin/ld.lld 
SUBLEVEL=0 EXTRAVERSION=-bisect ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- 
HOSTCC=/home/tcwg-buildslave/workspace/tcwg_kernel_0/llvm-install/bin/clang 
-j32 -s -k
  00:08:26 lib/test_scanf.c:661:2: error: implicit conversion from 'int' to 
'unsigned char' changes value from -168 to 88 [-Werror,-Wconstant-conversion]
  00:08:26   661 | test_number_prefix(unsigned char,   "0xA7", 
"%2hhx%hhx", 0, 0xa7, 2, check_uchar);
  00:08:26   | 
^
  00:08:26 lib/test_scanf.c:609:29: note: expanded from macro 
'test_number_prefix'
  00:08:26   609 | T result[2] = {~expect[0], ~expect[1]};  
   \
  00:08:26   |   ~^~
  00:08:27 1 error generated.
  00:08:27 make[3]: *** [scripts/Makefile.build:243: lib/test_scanf.o] Error 1

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/test_scanf.c#n661

Seems like the test should be fixed, though I wonder why this same error isn't 
effecting GCC builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76096

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

[PATCH] D146418: Support for OpenMP 5.0 sec 2.12.7 - Declare Target initializer expressions

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:23148
+  GlobalDeclRefChecker Checker;
+  if (auto *TargetVarDecl = dyn_cast_or_null(TargetDecl))
+Checker.declareTargetInitializer(TargetDecl);

FYI I fixed an unused var warning here: 
https://github.com/llvm/llvm-project/commit/f3ca99a87c566fb5e910071f4cbb474ddb4e7f37

Potentially you could pass the result of the cast into declareTargetInitializer 
but I don't know enough about the code to change that too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146418

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

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153430

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

[PATCH] D153430: [Clang][Driver] Warn on invalid Arm or AArch64 baremetal target triple

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> Does that make it clearer? (I still suspect that people won't understand what 
> is meant by "environment" here but I can't think of a better explanation).

"clang triple environment" into Google gets you the cross compilation guide and 
the Triple class docs so "environment" is useful here as one more keyword for 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153430

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

[PATCH] D153430: Warn on invalid Arm or AArch64 baremetal target triple

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> Perhaps "Unrecognized environment  for , did you mean 
> -none-"

I like explaining it like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153430

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

[PATCH] D153430: Warn on invalid Arm or AArch64 baremetal target triple

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Please tag the commit title with the subproject, `[clang]` in this case. If 
only to help your friendly neighborhood buildbot minder.

I'd maybe go with "Did you mean" instead of "try" but only because the first 
thing I think is well if I try it what will I get. Then again we're not going 
to fit an explanation of Arm triples into a warning message. I know elsewhere 
we already have "did you mean" but if "try" is already used too it's fine as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153430

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

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.



Comment at: clang/test/CodeGen/RISCV/riscv-inline-asm-gcc-commenting.c:23
+// CHECK: f1:
+// CHECK:  lui a0, 1
+// CHECK-NEXT: addiw   a0, a0, 564

Check for the comment content here too? `# this is fine # this should also be 
fine`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153008

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

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

Is the comment here relevant? 
https://discourse.llvm.org/t/interleaving-several-c-style-comments-in-the-same-inline-assembly-line/71353/8
 Does this patch do that already?

Also is it a problem that the ignored comments are not seen in the output? 
Perhaps you are just not checking for those bits. For comments on the end, 
those are propagated to the assembly output I know that much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153008

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

[PATCH] D152070: [2/11][Clang][RISCV] Expand all variants of RVV intrinsic tuple types

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

FYI after this change:

  Building CXX object 
tools/lldb/sou...luginTypeSystemClang.dir/TypeSystemClang.cpp.o
  
/home/david.spickett/llvm-project/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4843:13:
 warning: 225 enumeration values not handled in switch: 'RvvInt8mf8x2', 
'RvvInt8mf8x3', 'RvvInt8mf8x4'... [-Wswitch]
  switch (llvm::cast(qual_type)->getKind()) {
  ^~~~

lldb doesn't do anything with RVV yet, so you can likely just add all the names 
to the existing block of RVV stuff that just `break`s at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152070

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

[PATCH] D148783: [clangd] Add support TextDocumentEdit.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

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

[PATCH] D148783: [clangd] Add support TextDocumentEdit.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Hi, these tests are failing on our (Linaro's) Windows on Arm buildbot. First 
appearance here https://lab.llvm.org/buildbot/#/builders/65/builds/9346

Still failing as of a few hours ago 
https://lab.llvm.org/buildbot/#/builders/65/builds/9360 (maybe you have fixed 
this already, I am checking in infrequently over the next few days).

If you need any help debugging it let me know. It appears there is a drive 
letter on Windows:

  # CHECK-NEXT:  "uri": "file:///clangd-test/foo.c",
 ^
  :224:30: note: scanning from here
  "textDocument": {
   ^
  :225:15: note: possible intended match here
"uri": "file:///C:/clangd-test/foo.c",
^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148783

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

[PATCH] D148799: [clang] Return `std::string_view` from `TargetInfo::getClobbers()`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

If you have commit access already, wait until the pre-commit builds finish and 
check there isn't anything failing there.

If you don't, I can commit this for you as I did before. I'll do that on Monday.


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

https://reviews.llvm.org/D148799

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

[PATCH] D148799: [clang] Return `std::string_view` from `TargetInfo::getClobbers()`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Build issues will also show up in the "Build Status" here.


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

https://reviews.llvm.org/D148799

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

[PATCH] D148799: [clang] Return `std::string_view` from `TargetInfo::getClobbers()`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> I did not set const qualifier for return type because std::string_view is 
> constant by design. Or should I mark it const explicitly?

I'm new to string_view but everything I see backs up that it is a constant view 
on the data as you say. I'm not sure what making it itself const would do, not 
worth looking into here.

Please update the commit message (here and locally, in case you happen to be 
using `arc`, which prefers one and I forget which one). It should include the 
reasoning for the change, as we've discussed here.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:942
   std::string Constraints = "={@ccc},r,r,~{cc},~{memory}";
-  std::string MachineClobbers = CGF.getTarget().getClobbers();
+  std::string_view MachineClobbers CGF.getTarget().getClobbers();
   if (!MachineClobbers.empty()) {

You dropped a `=`, please make sure it compiles before updating the review.


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

https://reviews.llvm.org/D148799

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

[PATCH] D148799: [clang] Return `StringRef` from `TargetInfo::getClobbers()`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

string_view is a good call, especially as this is not doing anything fancy with 
it, and we have recently been allowed to use it.

> (Maybe) Make private static field of std::string type for actual clobber 
> value (or std::string_view type since it can be constexpr-constructed)

This I don't see the need for. If the string_views reference constant strings 
doesn't that amount to the same thing?

  const std::string_view getClobbers() const override {
  return "";
}

This makes a string view out of a pointer to a const string, but I guess it's 
up to the compiler how far it can optimise that.

You may be correct but it seems like a small optimisation.


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

https://reviews.llvm.org/D148799

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

[PATCH] D148799: [clang] Return `StringRef` from `TargetInfo::getClobbers()`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

You need to clearly state the "why" of the change in the commit message. 
Sometimes it seems obvious to the author but a reviewer has to assume and may 
assume incorrectly.

In this case, I guess that since the result of the function only has its length 
checked then is added to another string, you would avoid a heap allocation for 
the new std::string temporary by using StringRef.

You could also do this by making a StringRef from the const char* and checking 
its length. That does leave a small ambiguity as to whether returning nullptr 
and empty string is the same thing. Which it is, but it's not clear just from 
the prototype of the function. So changing getClobbers to return StringRef 
resolves both issues.

But equally, you might have had a completely different goal. Tell me if I am 
right :)

> Offtop: Is there any discussion platform where I can share some ideas? I have 
> some of them how to refactor TargetInfo classes, but it's a major (and maybe 
> breaking) change I have to discuss prior to implement.

An RFC on discourse is the usual way. If you can supply some work in progress 
patches or a tree on github then even better (and prevent you from proposing 
things you later find to be impossible, not that I have ever done that of 
course :) ).


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

https://reviews.llvm.org/D148799

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

[PATCH] D148799: [clang] Return `StringRef` from `TargetInfo::getClobbers()`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> Context not available.

Please update the diff with context, 
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

I'm not sure what the benefit of this change is. While I did say less raw 
pointers = better, perhaps I'd make an exception for `const char *` for 
constant strings. In this case, no one is manipulating the string until (I 
assume) after it's converted to std::string, so we're not removing risky 
accesses in that way. If the API were returning a `const char *` that we then 
did a bunch of find first, split at, strlen, etc on, then this would make more 
sense.

https://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes

Also this says you can implicitly construct from a string literal, so I am 
surprised you need the `llvm::StringLiteral`. Unless this is just a thing that 
asserts that it is in fact, a literal (I've not used this before myself).

Overall I applaud the effort but in this particular case it may not be needed.


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

https://reviews.llvm.org/D148799

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

[PATCH] D148574: [clang] Return std::unique_ptr from AllocateTarget

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> What should I do further?

And contribute more if you like :)

Thanks for the contribution!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148574

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

[PATCH] D148574: [clang] Return std::unique_ptr from AllocateTarget

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

You may get some emails about build failures. Try to judge if they are relevant 
to you, ask here if you are not sure.

I doubt it will do much given that it's just refactoring and both uses seem to 
be GPU offload targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148574

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

[PATCH] D148574: [clang] Return std::unique_ptr from AllocateTarget

[David Spickett via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG830b359d3ab1: [clang] Return 
std::unique_ptr from AllocateTarget (authored by Stoorx, 
committed by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148574

Files:
  clang/lib/Basic/Targets.cpp
  clang/lib/Basic/Targets.h
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/Basic/Targets/SPIR.h

Index: clang/lib/Basic/Targets/SPIR.h
===
--- clang/lib/Basic/Targets/SPIR.h
+++ clang/lib/Basic/Targets/SPIR.h
@@ -111,7 +111,7 @@
 llvm::Triple HostTriple(Opts.HostTriple);
 if (!HostTriple.isSPIR() && !HostTriple.isSPIRV() &&
 HostTriple.getArch() != llvm::Triple::UnknownArch) {
-  HostTarget.reset(AllocateTarget(llvm::Triple(Opts.HostTriple), Opts));
+  HostTarget = AllocateTarget(llvm::Triple(Opts.HostTriple), Opts);
 
   // Copy properties from host target.
   BoolWidth = HostTarget->getBoolWidth();
Index: clang/lib/Basic/Targets/NVPTX.cpp
===
--- clang/lib/Basic/Targets/NVPTX.cpp
+++ clang/lib/Basic/Targets/NVPTX.cpp
@@ -73,7 +73,7 @@
   // types.
   llvm::Triple HostTriple(Opts.HostTriple);
   if (!HostTriple.isNVPTX())
-HostTarget.reset(AllocateTarget(llvm::Triple(Opts.HostTriple), Opts));
+HostTarget = AllocateTarget(llvm::Triple(Opts.HostTriple), Opts);
 
   // If no host target, make some guesses about the data layout and return.
   if (!HostTarget) {
Index: clang/lib/Basic/Targets.h
===
--- clang/lib/Basic/Targets.h
+++ clang/lib/Basic/Targets.h
@@ -24,8 +24,8 @@
 namespace targets {
 
 LLVM_LIBRARY_VISIBILITY
-clang::TargetInfo *AllocateTarget(const llvm::Triple &Triple,
-  const clang::TargetOptions &Opts);
+std::unique_ptr
+AllocateTarget(const llvm::Triple &Triple, const clang::TargetOptions &Opts);
 
 /// DefineStd - Define a macro name and standard variants.  For example if
 /// MacroName is "unix", then this will define "__unix", "__unix__", and "unix"
Index: clang/lib/Basic/Targets.cpp
===
--- clang/lib/Basic/Targets.cpp
+++ clang/lib/Basic/Targets.cpp
@@ -108,8 +108,8 @@
 // Driver code
 //===--===//
 
-TargetInfo *AllocateTarget(const llvm::Triple &Triple,
-   const TargetOptions &Opts) {
+std::unique_ptr AllocateTarget(const llvm::Triple &Triple,
+   const TargetOptions &Opts) {
   llvm::Triple::OSType os = Triple.getOS();
 
   switch (Triple.getArch()) {
@@ -117,159 +117,171 @@
 return nullptr;
 
   case llvm::Triple::arc:
-return new ARCTargetInfo(Triple, Opts);
+return std::make_unique(Triple, Opts);
 
   case llvm::Triple::xcore:
-return new XCoreTargetInfo(Triple, Opts);
+return std::make_unique(Triple, Opts);
 
   case llvm::Triple::hexagon:
 if (os == llvm::Triple::Linux &&
 Triple.getEnvironment() == llvm::Triple::Musl)
-  return new LinuxTargetInfo(Triple, Opts);
-return new HexagonTargetInfo(Triple, Opts);
+  return std::make_unique>(Triple, Opts);
+return std::make_unique(Triple, Opts);
 
   case llvm::Triple::lanai:
-return new LanaiTargetInfo(Triple, Opts);
+return std::make_unique(Triple, Opts);
 
   case llvm::Triple::aarch64_32:
 if (Triple.isOSDarwin())
-  return new DarwinAArch64TargetInfo(Triple, Opts);
+  return std::make_unique(Triple, Opts);
 
 return nullptr;
   case llvm::Triple::aarch64:
 if (Triple.isOSDarwin())
-  return new DarwinAArch64TargetInfo(Triple, Opts);
+  return std::make_unique(Triple, Opts);
 
 switch (os) {
 case llvm::Triple::CloudABI:
-  return new CloudABITargetInfo(Triple, Opts);
+  return std::make_unique>(Triple,
+   Opts);
 case llvm::Triple::FreeBSD:
-  return new FreeBSDTargetInfo(Triple, Opts);
+  return std::make_unique>(Triple,
+  Opts);
 case llvm::Triple::Fuchsia:
-  return new FuchsiaTargetInfo(Triple, Opts);
+  return std::make_unique>(Triple,
+  Opts);
 case llvm::Triple::Linux:
   switch (Triple.getEnvironment()) {
   default:
-return new LinuxTargetInfo(Triple, Opts);
+return std::make_unique>(Triple,
+  Opts);
   case llvm::Triple::OpenHOS:
-return new OHOSTargetInfo(Triple, Opts);
+return std

[PATCH] D148574: [clang] Return std::unique_ptr from AllocateTarget

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

> Or should I provide a real name?

Username is fine as long as you're fine being credited as such.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148574

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

[PATCH] D148574: [clang] Use the 'std::unique_ptr' instead of a raw pointers

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I'd suggest rewording the title a bit: `[clang] Return 
std::unique_ptr from AllocateTarget`

Titles are always weird but at least including the function name means someone 
staring at a build failure can skim for likely commits. It's not always 
possible though.

Same results for me without this change:

  Failed Tests (6):
LLVM :: Examples/OrcV2Examples/lljit-with-remote-debugging.test
LLVM :: Examples/OrcV2Examples/lljit-with-thinlto-summaries.test
LLVM :: Examples/OrcV2Examples/orcv2-cbindings-add-object-file.test
LLVM :: Examples/OrcV2Examples/orcv2-cbindings-basic-usage.test
LLVM :: Examples/OrcV2Examples/orcv2-cbindings-lazy.test
LLVM :: Examples/OrcV2Examples/orcv2-cbindings-removable-code.test

The examples are not built by default you have to give `LLVM_BUILD_EXAMPLES=ON` 
to CMake.

It's not your problem to fix those, this patch doesn't cause them so it's fine 
to go in as is.

Do you have commit access? 
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

If you want I can land this for you, I need a name and email address to use for 
the commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148574

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

[PATCH] D148574: [clang] Use the 'std::unique_ptr' instead of a raw pointers

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> That's my first commit into LLVM project. (Almost just for test of the 
> contribution procedure.)

Welcome!

> Maybe I did something wrong, but the build fails on example tests.

Do the tests fail locally? I will look at the pre-commit logs as well. It may 
be failures that were fixed already and this change needs to be rebased to 
include that.

> I wonder, whether this review request will be actually reviewed despite the 
> failed condition? And maybe someone can provide some (simple) 
> tutorials/guides how to contribute here in more correct way.

Yes, absolutely review isn't just about criticising code it's about solving 
problems.

The intent of this change is great (down with raw pointers) and at first glance 
I don't think it should cause any failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148574

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

[PATCH] D148529: [clang] Replace find_executable with shutil.which in creduce script

[David Spickett via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGee3413736251: [clang] Replace find_executable with 
shutil.which in creduce script (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148529

Files:
  clang/utils/creduce-clang-crash.py


Index: clang/utils/creduce-clang-crash.py
===
--- clang/utils/creduce-clang-crash.py
+++ clang/utils/creduce-clang-crash.py
@@ -11,6 +11,7 @@
 from argparse import ArgumentParser, RawTextHelpFormatter
 import os
 import re
+import shutil
 import stat
 import sys
 import subprocess
@@ -18,7 +19,6 @@
 import shlex
 import tempfile
 import shutil
-from distutils.spawn import find_executable
 import multiprocessing
 
 verbose = False
@@ -43,12 +43,12 @@
   if cmd_path:
 # Make the path absolute so the creduce test can be run from any directory.
 cmd_path = os.path.abspath(cmd_path)
-cmd = find_executable(cmd_path)
+cmd = shutil.which(cmd_path)
 if cmd:
   return cmd
 sys.exit("ERROR: executable `%s` not found" % (cmd_path))
 
-  cmd = find_executable(cmd_name, path=cmd_dir)
+  cmd = shutil.which(cmd_name, path=cmd_dir)
   if cmd:
 return cmd
 


Index: clang/utils/creduce-clang-crash.py
===
--- clang/utils/creduce-clang-crash.py
+++ clang/utils/creduce-clang-crash.py
@@ -11,6 +11,7 @@
 from argparse import ArgumentParser, RawTextHelpFormatter
 import os
 import re
+import shutil
 import stat
 import sys
 import subprocess
@@ -18,7 +19,6 @@
 import shlex
 import tempfile
 import shutil
-from distutils.spawn import find_executable
 import multiprocessing
 
 verbose = False
@@ -43,12 +43,12 @@
   if cmd_path:
 # Make the path absolute so the creduce test can be run from any directory.
 cmd_path = os.path.abspath(cmd_path)
-cmd = find_executable(cmd_path)
+cmd = shutil.which(cmd_path)
 if cmd:
   return cmd
 sys.exit("ERROR: executable `%s` not found" % (cmd_path))
 
-  cmd = find_executable(cmd_name, path=cmd_dir)
+  cmd = shutil.which(cmd_name, path=cmd_dir)
   if cmd:
 return cmd
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148529: [clang] Replace find_executable with shutil.which in creduce script

[David Spickett via Phabricator via cfe-commits]

DavidSpickett created this revision.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

distutils is deprecated and shutil.which is the suggested
replacement for this function.

https://peps.python.org/pep-0632/#migration-advice
https://docs.python.org/3/library/shutil.html#shutil.which

which was added in 3.3 
(https://docs.python.org/3/library/shutil.html#shutil.which)
and LLVM requires at least 3.6 
(https://llvm.org/docs/GettingStarted.html#software).

There is one small differnce here that shutil.which ignores the PATH
when given a path argument. However in this case I think that's actually
the behaviour we want.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148529

Files:
  clang/utils/creduce-clang-crash.py


Index: clang/utils/creduce-clang-crash.py
===
--- clang/utils/creduce-clang-crash.py
+++ clang/utils/creduce-clang-crash.py
@@ -11,6 +11,7 @@
 from argparse import ArgumentParser, RawTextHelpFormatter
 import os
 import re
+import shutil
 import stat
 import sys
 import subprocess
@@ -18,7 +19,6 @@
 import shlex
 import tempfile
 import shutil
-from distutils.spawn import find_executable
 import multiprocessing
 
 verbose = False
@@ -43,12 +43,12 @@
   if cmd_path:
 # Make the path absolute so the creduce test can be run from any directory.
 cmd_path = os.path.abspath(cmd_path)
-cmd = find_executable(cmd_path)
+cmd = shutil.which(cmd_path)
 if cmd:
   return cmd
 sys.exit("ERROR: executable `%s` not found" % (cmd_path))
 
-  cmd = find_executable(cmd_name, path=cmd_dir)
+  cmd = shutil.which(cmd_name, path=cmd_dir)
   if cmd:
 return cmd
 


Index: clang/utils/creduce-clang-crash.py
===
--- clang/utils/creduce-clang-crash.py
+++ clang/utils/creduce-clang-crash.py
@@ -11,6 +11,7 @@
 from argparse import ArgumentParser, RawTextHelpFormatter
 import os
 import re
+import shutil
 import stat
 import sys
 import subprocess
@@ -18,7 +19,6 @@
 import shlex
 import tempfile
 import shutil
-from distutils.spawn import find_executable
 import multiprocessing
 
 verbose = False
@@ -43,12 +43,12 @@
   if cmd_path:
 # Make the path absolute so the creduce test can be run from any directory.
 cmd_path = os.path.abspath(cmd_path)
-cmd = find_executable(cmd_path)
+cmd = shutil.which(cmd_path)
 if cmd:
   return cmd
 sys.exit("ERROR: executable `%s` not found" % (cmd_path))
 
-  cmd = find_executable(cmd_name, path=cmd_dir)
+  cmd = shutil.which(cmd_name, path=cmd_dir)
   if cmd:
 return cmd
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

cfe-commits@lists.llvm.org

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I've reverted this, please try to fix the test then reland.

The full test output can be downloaded from the buildbot page, if you need any 
more information let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143128

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

cfe-commits@lists.llvm.org

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

The test is also failing on our Windows on Arm bot: 
https://lab.llvm.org/buildbot/#/builders/65/builds/8950


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143128

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

[PATCH] D143704: [flang] Feature list plugin

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Specifically https://lab.llvm.org/buildbot/#/builders/181/builds/15552. Though 
it is only the sharedlibs build failing, I don't know why.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143704

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

[PATCH] D146041: Fix weirdly apologetic diagnostic messages

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

> Since this is my first commit to such a large repository(and project), can 
> you please guide me with this @DavidSpickett !

Sure, you'll want to make a commit that only has changes to warnings and 
errors. You can split up this one to do that, see part "A)" of this answer 
https://stackoverflow.com/questions/6217156/break-a-previous-commit-into-multiple-commits/6217314#6217314.
 Then you can update this review with that new commit, change the 
description/title etc. if needed.

If you get confused with updating the review (happens to me all the time) you 
can just abandon this (there is an entry in the "Add Action..." menu) and make 
a new review as you did before.

How to identify what changes should be included? I would ignore comments, shell 
scripts, FIXMEs, or general test data. If the test is producing a warning and 
looking for it, clearly it should be changed. If it's just random data it's 
using to test some function, I wouldn't change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146041

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

[PATCH] D146041: Fix weirdly apologetic diagnostic messages

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Ah, here it is: 
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

Seems to back you up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146041

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

[PATCH] D146041: Fix weirdly apologetic diagnostic messages

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Is there some standard for writing warning messages? For llvm that is, it would 
be worth looking through the getting started guides to see. I think the 
majority of warnings are "formal" in that sense so this seems fine.

Personally I agree with making the warnings more succinct but aside from that I 
don't see the need to change comments or testing scripts.

You may consider splitting this change into 2. One that only changes warnings 
and errors (a less controversial change) and the rest (that is up to the 
reviewers of each bit).




Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:51
 template<_Complex int> struct ComplexInt {};
-using CI = ComplexInt<1 + 3i>; // FIXME: expected-error {{sorry}}
-using CI = ComplexInt<1 + 3i>; // FIXME: expected-error {{sorry}}

Please find out what the {{sorry}} here was for. Did someone literally write it 
as an apology, or is it indicating some warning that should be emitted here, if 
the FIXME is fixed?

You could git blame this file and find the commit that introduced it, as a 
starting point.



Comment at: polly/lib/External/isl/imath/tests/test.sh:12
 if [ ! -f ../imtest ] ; then
-  echo "I can't find the imath test driver 'imtest', did you build it?"
-  echo "I can't proceed with the unit tests until you do so, sorry."
+  echo "The imath test driver 'imtest' was not found."
+  echo "It needs to be build before proceeding with the unit tests."

Given that this is a developer facing script, I think the "did you build it" is 
in fact quite useful here. Perhaps it could say:
"The imath test driver 'imtest' was not found. Did you build it? It is required 
for running the tests."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146041

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

[PATCH] D145227: [LLVM][OHOS] Clang toolchain and targets

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Also failing on our Windows on Arm bot: 
https://lab.llvm.org/buildbot/#/builders/65/builds/8607

I think some lines need to account for Windows slashes, `{{/|}}` should do 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145227

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

[PATCH] D145227: [LLVM][OHOS] Clang toolchain and targets

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

Looks good in general. If there are some incorrect details you'll find them 
later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145227

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

[PATCH] D145781: [AArch64] Don't #define __ARM_FP when there's no FPU.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145781

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

[PATCH] D145227: [LLVM][OHOS] Clang toolchain and targets

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I'm not familiar with the details of toolchain config, but added some general 
comments.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1404
+  // OHOS-specific defaults for PIC/PIE
+  if (Triple.isOHOSFamily()) {
+switch (Triple.getArch()) {

Collapse this into `if isohos && triple.getarch is...`.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1702
+  if (TC.getTriple().isOHOSFamily() && UNW != ToolChain::UNW_None) {
+CmdArgs.push_back("-l:libunwind.a");
+return;

Please add a comment explaining this. Even if it is just "OHOS is only 
compatible with libunwind". At least then we know it's nothing more mysterious.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:645
 
-  if (WantPthread && !isAndroid)
+  // We don't need libpthread neither for bionic nor for musl
+  if (WantPthread && !isAndroid && !isOHOSFamily)

And musl is what OHOS uses? Please add that to the comment if so "for musl, 
which OHOS uses...".



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2318
+  "mipsel-linux-gnu", "mips-img-linux-gnu", "mipsisa32r6el-linux-gnu",
+  "mipsel-linux-ohos"};
 

No riscv related changes needed in this file?



Comment at: clang/lib/Driver/ToolChains/OHOS.cpp:164
+  // For compatibility with arm-liteos sysroot
+  // FIXME: Remove this when we'll use arm-liteos sysroot produced by build.py.
+  addPathIfExists(

Keep FIXMEs that refer to stuff outside the project downstream please :)



Comment at: clang/lib/Driver/ToolChains/OHOS.cpp:372
+// FIXME: gnu or both???
+CmdArgs.push_back("--hash-style=both");
+  }

Maybe this helps? 
https://github.com/llvm/llvm-project/blob/e00c73c856a325008afead10cfb3e9d0fc4a1e41/clang/lib/Driver/ToolChains/Linux.cpp#L235

(no idea myself)



Comment at: clang/test/Driver/ohos.c:240
+
+// CHECK-OHOS-PTHREAD-NOT: -lpthread
+

This one is checking that we do not link to a pthread library, because when 
using musl, it already provides one?

Just checking you're not accepting the option but doing the opposite here. 
Worth adding a comment to explain the reasoning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145227

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

[PATCH] D135495: [clang-tidy] handle exceptions properly in `ExceptionAnalyzer`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

If it helps, the bot I linked is pretty vanilla. You can find all the details 
in the buildbot UI but just in case:

  cmake -G Ninja ../llvm/llvm -DCMAKE_BUILD_TYPE=Release 
-DLLVM_ENABLE_ASSERTIONS=True '-DLLVM_LIT_ARGS='"'"'-v'"'"'' 
-DCMAKE_INSTALL_PREFIX=../stage1.install 
'-DLLVM_TARGETS_TO_BUILD='"'"'AArch64'"'"'' 
'-DLLVM_ENABLE_PROJECTS=llvm;clang;clang-tools-extra'

We build with clang 15.0.0 release. Though I guess it's not a compiler related 
issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135495

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

[PATCH] D135495: [clang-tidy] handle exceptions properly in `ExceptionAnalyzer`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

The test was timing out on Arm/AArch64 bots so I have reverted this change.

I think because there were other test failures on the initial run, that 
obscured the time out. This is one of the first builds to include this change: 
https://lab.llvm.org/buildbot/#/builders/188/builds/26480 Note that there is an 
unrelated failure, and the stdio shows a timeout.

  PASS: lit :: shtest-define.py (70393 of 70394)
  command timed out: 1200 seconds without output running [b'ninja', 
b'check-all'], attempting to kill

I confirmed that on the bot itself, it was exception-escape that was holding 
things up.

If this is not happening outside of Arm/AArch64 I can help you understand what 
went wrong (though I will be finishing work for the week shortly, back Monday).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135495

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

[PATCH] D143052: [CMake] Replace llvm_check_linker_flag and llvm_check_compiler_linker_flag

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.

https://cmake.org/cmake/help/v3.18/module/CheckLinkerFlag.html

Looks equivalent to me, good timing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143052

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

On AArch64 we have the following failures:

  Failed Tests (15):
Clang :: CXX/basic/basic.stc/basic.stc.dynamic/p2-nodef.cpp
Clang :: CodeCompletion/ordinary-name-cxx11.cpp
Clang :: CodeCompletion/ordinary-name.cpp
Clang :: CodeGenCXX/cxx20-module-std-subst-2b.cpp
Clang :: CodeGenCXX/cxx20-module-std-subst-2c.cpp
Clang :: Index/complete-preamble.cpp
Clang :: Index/load-namespaces.cpp
Clang :: Modules/implicit-declared-allocation-functions.cppm
Clang :: PCH/chain-friend-instantiation.cpp
Clang :: PCH/cxx1z-aligned-alloc.cpp
Clang :: SemaCXX/constructor-initializer.cpp
Clang :: SemaCXX/using-directive.cpp
Clang-Unit :: 
AST/./ASTTests/ParameterizedTests/DeclContextTest/removeDeclOfClassTemplateSpecialization/0
Clang-Unit :: 
StaticAnalyzer/./StaticAnalysisTests/CallDescription/RejectOverQualifiedNames
Clang-Unit :: 
StaticAnalyzer/./StaticAnalysisTests/CallDescription/SkipAnonimousNamespaces

None of which are caused by assertions but instead errors of the form:

  input.cc:12:7: error: reference to 'std' is ambiguous
std::container v;
^
  note: candidate found by name lookup is 'std'
  input.cc:3:15: note: candidate found by name lookup is 'my::std'
  namespace std {
^
  1 error generated.

  
/home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:14:18:
 error: reference to 'std' is ambiguous
  void f(str> &s) {
   ^
  
/home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:7:11:
 note: candidate found by name lookup is 'std'
  namespace std {
^

On Arm it's the exact same set of failures.

What would help you debug those? I don't know the background for the change but 
I can check specifics on the machine itself if needed.

Test log from AArch64: F26314570: tests.log 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcd173cbd7cca: [clang][compiler-rt] Support 
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux… (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140011

Files:
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-pc-windows-msvc/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabihf/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabihf/.keep
  clang/test/Driver/arm-float-abi-runtime-path.c
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  llvm/CMakeLists.txt

Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -817,8 +817,7 @@
 set(LLVM_TARGET_TRIPLE_ENV CACHE STRING "The name of environment variable to override default target. Disabled by blank.")
 mark_as_advanced(LLVM_TARGET_TRIPLE_ENV)
 
-# Per target dir not yet supported on Arm 32 bit due to arm vs armhf handling
-if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux" AND NOT CMAKE_SYSTEM_PROCESSOR MATCHES "^arm")
+if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux")
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)
 else()
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF)
Index: compiler-rt/cmake/Modules/CompilerRTUtils.cmake
===
--- compiler-rt/cmake/Modules/CompilerRTUtils.cmake
+++ compiler-rt/cmake/Modules/CompilerRTUtils.cmake
@@ -433,6 +433,25 @@
 string(REGEX REPLACE "mipsisa64" "mipsisa32" triple_cpu_mips "${triple_cpu}")
 string(REGEX REPLACE "mips64" "mips" triple_cpu_mips "${triple_cpu_mips}")
 set(target "${triple_cpu_mips}${triple_suffix_gnu}")
+  elseif("${arch}" MATCHES "^arm")
+# Arch is arm, armhf, armv6m (anything else would come from using
+# COMPILER_RT_DEFAULT_TARGET_ONLY, which is checked above).
+if (${arch} STREQUAL "armhf")
+  # If we are building for hard float but our ABI is soft float.
+  if ("${triple_suffix}" MATCHES ".*eabi$")
+# Change "eabi" -> "eabihf"
+set(triple_suffix "${triple_suffix}hf")
+  endif()
+  # ABI is already set in the triple, don't repeat it in the architecture.
+  set(arch "arm")
+else ()
+  # If we are building for soft float, but the triple's ABI is hard float.
+  if ("${triple_suffix}" MATCHES ".*eabihf$")
+# Change "eabihf" -> "eabi"
+string(REGEX REPLACE "hf$" "" triple_suffix "${triple_suffix}")
+  endif()
+endif()
+set(target "${arch}${triple_suffix}")
   else()
 set(target "${arch}${triple_suffix}")
   endif()
Index: clang/test/Driver/arm-float-abi-runtime-path.c
===
--- /dev/null
+++ clang/test/Driver/arm-float-abi-runtime-path.c
@@ -0,0 +1,33 @@
+/// Check that libraries built with the per target runtime directory layout
+/// are selected correctly when using variations of Arm triples.
+
+// REQUIRES: arm-registered-target
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+/// "armv7l" should be normalised to just "arm".
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+
+/// armeb triples should be unmodified.
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEBHF %s
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEB %s
+
+// RUN: %clang %s --target=arm-pc-windows-msvc -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=WINDOWS %s
+/// armhf-pc... isn't recognised so just check that the float-abi option is ignored
+// RUN: %clang %s --target=arm-pc-windows-msvc -mfloat-abi=hard -pr

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

DavidSpickett updated this revision to Diff 493518.
DavidSpickett added a comment.

Remove the Armhf comment, rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140011

Files:
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-pc-windows-msvc/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabihf/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabihf/.keep
  clang/test/Driver/arm-float-abi-runtime-path.c
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  llvm/CMakeLists.txt

Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -817,8 +817,7 @@
 set(LLVM_TARGET_TRIPLE_ENV CACHE STRING "The name of environment variable to override default target. Disabled by blank.")
 mark_as_advanced(LLVM_TARGET_TRIPLE_ENV)
 
-# Per target dir not yet supported on Arm 32 bit due to arm vs armhf handling
-if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux" AND NOT CMAKE_SYSTEM_PROCESSOR MATCHES "^arm")
+if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux")
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)
 else()
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF)
Index: compiler-rt/cmake/Modules/CompilerRTUtils.cmake
===
--- compiler-rt/cmake/Modules/CompilerRTUtils.cmake
+++ compiler-rt/cmake/Modules/CompilerRTUtils.cmake
@@ -433,6 +433,25 @@
 string(REGEX REPLACE "mipsisa64" "mipsisa32" triple_cpu_mips "${triple_cpu}")
 string(REGEX REPLACE "mips64" "mips" triple_cpu_mips "${triple_cpu_mips}")
 set(target "${triple_cpu_mips}${triple_suffix_gnu}")
+  elseif("${arch}" MATCHES "^arm")
+# Arch is arm, armhf, armv6m (anything else would come from using
+# COMPILER_RT_DEFAULT_TARGET_ONLY, which is checked above).
+if (${arch} STREQUAL "armhf")
+  # If we are building for hard float but our ABI is soft float.
+  if ("${triple_suffix}" MATCHES ".*eabi$")
+# Change "eabi" -> "eabihf"
+set(triple_suffix "${triple_suffix}hf")
+  endif()
+  # ABI is already set in the triple, don't repeat it in the architecture.
+  set(arch "arm")
+else ()
+  # If we are building for soft float, but the triple's ABI is hard float.
+  if ("${triple_suffix}" MATCHES ".*eabihf$")
+# Change "eabihf" -> "eabi"
+string(REGEX REPLACE "hf$" "" triple_suffix "${triple_suffix}")
+  endif()
+endif()
+set(target "${arch}${triple_suffix}")
   else()
 set(target "${arch}${triple_suffix}")
   endif()
Index: clang/test/Driver/arm-float-abi-runtime-path.c
===
--- /dev/null
+++ clang/test/Driver/arm-float-abi-runtime-path.c
@@ -0,0 +1,33 @@
+/// Check that libraries built with the per target runtime directory layout
+/// are selected correctly when using variations of Arm triples.
+
+// REQUIRES: arm-registered-target
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+/// "armv7l" should be normalised to just "arm".
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+
+/// armeb triples should be unmodified.
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEBHF %s
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEB %s
+
+// RUN: %clang %s --target=arm-pc-windows-msvc -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=WINDOWS %s
+/// armhf-pc... isn't recognised so just check that the float-abi option is ignored
+// RUN: %clang %s --target=arm-pc-windows-msvc -mfloat-abi=hard -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=WINDOWS %s
+
+// ARMHF:   lib{{/|\\}

[PATCH] D141796: [15/15][Clang][RISCV][NFC] Set data member under Policy as constants

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Looks good to me - https://lab.llvm.org/buildbot/#/builders/245/builds/3919


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141796

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

[PATCH] D141796: [15/15][Clang][RISCV][NFC] Set data member under Policy as constants

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Nice!

Glad you spotted that, I have had creduce running and getting nowhere fast for 
a day or so. Sounds like it would never have found anything minimal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141796

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I am attempting to reduce the compiler crash on 32 bit, it's a slow process.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D141796: [15/15][Clang][RISCV][NFC] Set data member under Policy as constants

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Hi! Comparing the changes for 
https://lab.llvm.org/buildbot/#/builders/17/builds/33129 and 
https://lab.llvm.org/buildbot/#/builders/174/builds/17069 I think you've 
managed to hit a crash in the host compiler we (Linaro) are using for our bots. 
Which is the 15.0.0 release.

Either way I will handle it, perhaps I can reduce it down to some pattern 15.x 
doesn't like but either way I don't think this change is at fault.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141796

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I was able to reproduce on AArch64 and Arm (32 bit), did not reproduce on an 
Intel machine which matches what you've seen so far. 62 failures on AArch64, 21 
on Arm. Broadly two kinds of failure.

Errors of the form `error: reference to 'std' is ambiguous`, which happens on 
both platforms.

Asserts, which only show up on AArch64:

  clang: 
/home/david.spickett/llvm-project/clang/lib/Serialization/ASTWriter.cpp:5455: 
clang::serialization::DeclID clang::ASTWriter::getDeclID(const clang::Decl *): 
Assertion `DeclIDs.find(D) != DeclIDs.end() && "Declaration not emitted!"' 
failed.

If you have some inclination of what might be going on I can check for specific 
things. Failing that I can get you access to a development container to try it 
yourself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Hi folks, I'm one of the owners of the AArch64 bot you're seeing the failure 
on. I will try to reproduce it myself and see if I can find what's going on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I will wait until after the 16 branch happens to land this. The release build 
already has some issues and I won't have time to do the fixups in the next few 
weeks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140011

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

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

DavidSpickett updated this revision to Diff 483112.
DavidSpickett added a comment.

Expand the comment and show an example of changed triples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140011

Files:
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-pc-windows-msvc/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabihf/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabihf/.keep
  clang/test/Driver/arm-float-abi-runtime-path.c
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  llvm/CMakeLists.txt

Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -816,7 +816,7 @@
 mark_as_advanced(LLVM_TARGET_TRIPLE_ENV)
 
 # Per target dir not yet supported on Arm 32 bit due to arm vs armhf handling
-if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux" AND NOT CMAKE_SYSTEM_PROCESSOR MATCHES "^arm")
+if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux")
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)
 else()
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF)
Index: compiler-rt/cmake/Modules/CompilerRTUtils.cmake
===
--- compiler-rt/cmake/Modules/CompilerRTUtils.cmake
+++ compiler-rt/cmake/Modules/CompilerRTUtils.cmake
@@ -433,6 +433,25 @@
 string(REGEX REPLACE "mipsisa64" "mipsisa32" triple_cpu_mips "${triple_cpu}")
 string(REGEX REPLACE "mips64" "mips" triple_cpu_mips "${triple_cpu_mips}")
 set(target "${triple_cpu_mips}${triple_suffix_gnu}")
+  elseif("${arch}" MATCHES "^arm")
+# Arch is arm, armhf, armv6m (anything else would come from using
+# COMPILER_RT_DEFAULT_TARGET_ONLY, which is checked above).
+if (${arch} STREQUAL "armhf")
+  # If we are building for hard float but our ABI is soft float.
+  if ("${triple_suffix}" MATCHES ".*eabi$")
+# Change "eabi" -> "eabihf"
+set(triple_suffix "${triple_suffix}hf")
+  endif()
+  # ABI is already set in the triple, don't repeat it in the architecture.
+  set(arch "arm")
+else ()
+  # If we are building for soft float, but the triple's ABI is hard float.
+  if ("${triple_suffix}" MATCHES ".*eabihf$")
+# Change "eabihf" -> "eabi"
+string(REGEX REPLACE "hf$" "" triple_suffix "${triple_suffix}")
+  endif()
+endif()
+set(target "${arch}${triple_suffix}")
   else()
 set(target "${arch}${triple_suffix}")
   endif()
Index: clang/test/Driver/arm-float-abi-runtime-path.c
===
--- /dev/null
+++ clang/test/Driver/arm-float-abi-runtime-path.c
@@ -0,0 +1,33 @@
+/// Check that libraries built with the per target runtime directory layout
+/// are selected correctly when using variations of Arm triples.
+
+// REQUIRES: arm-registered-target
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+/// "armv7l" should be normalised to just "arm".
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+
+/// armeb triples should be unmodified.
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEBHF %s
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEB %s
+
+// RUN: %clang %s --target=arm-pc-windows-msvc -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=WINDOWS %s
+/// armhf-pc... isn't recognised so just check that the float-abi option is ignored
+// RUN: %clang %s --target=arm-pc-windows-msvc -mfloat-abi=hard -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=WINDOWS %s
+
+// ARMHF:   lib{{/|\\}}arm-unknown-linux-gnueabihf{{$}}
+// ARM: lib{{/|\\}}arm-unknown-linux-gnueabi{{$}}
+// ARMEBH

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I haven't tested this on a BSD myself. I don't see a major reason it wouldn't 
work but if you think it's going to have issues I can do that testing and/or 
restrict this change to Linux only initially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140011

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

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

DavidSpickett updated this revision to Diff 482811.
DavidSpickett added a comment.

- `-target` changed to `--target=` in test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140011

Files:
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-pc-windows-msvc/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabihf/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabihf/.keep
  clang/test/Driver/arm-float-abi-runtime-path.c
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  llvm/CMakeLists.txt

Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -816,7 +816,7 @@
 mark_as_advanced(LLVM_TARGET_TRIPLE_ENV)
 
 # Per target dir not yet supported on Arm 32 bit due to arm vs armhf handling
-if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux" AND NOT CMAKE_SYSTEM_PROCESSOR MATCHES "^arm")
+if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux")
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)
 else()
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF)
Index: compiler-rt/cmake/Modules/CompilerRTUtils.cmake
===
--- compiler-rt/cmake/Modules/CompilerRTUtils.cmake
+++ compiler-rt/cmake/Modules/CompilerRTUtils.cmake
@@ -433,6 +433,25 @@
 string(REGEX REPLACE "mipsisa64" "mipsisa32" triple_cpu_mips "${triple_cpu}")
 string(REGEX REPLACE "mips64" "mips" triple_cpu_mips "${triple_cpu_mips}")
 set(target "${triple_cpu_mips}${triple_suffix_gnu}")
+  elseif("${arch}" MATCHES "^arm")
+# Arch is arm, armhf, armv6m (anything else would come from using
+# COMPILER_RT_DEFAULT_TARGET_ONLY, which is checked above).
+if (${arch} STREQUAL "armhf")
+  # If we are building for hard float but our ABI is soft float.
+  if ("${triple_suffix}" MATCHES ".*eabi$")
+# Change "eabi" -> "eabihf"
+set(triple_suffix "${triple_suffix}hf")
+  endif()
+  # ABI is already set in the triple, don't repeat it in the architecture.
+  set(arch "arm")
+else ()
+  # If we are building for soft float, but the triple's ABI is hard float.
+  if ("${triple_suffix}" MATCHES ".*eabihf$")
+# Change "eabihf" -> "eabi"
+string(REGEX REPLACE "hf$" "" triple_suffix "${triple_suffix}")
+  endif()
+endif()
+set(target "${arch}${triple_suffix}")
   else()
 set(target "${arch}${triple_suffix}")
   endif()
Index: clang/test/Driver/arm-float-abi-runtime-path.c
===
--- /dev/null
+++ clang/test/Driver/arm-float-abi-runtime-path.c
@@ -0,0 +1,33 @@
+/// Check that libraries built with the per target runtime directory layout
+/// are selected correctly when using variations of Arm triples.
+
+// REQUIRES: arm-registered-target
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+/// "armv7l" should be normalised to just "arm".
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMHF %s
+
+// RUN: %clang %s --target=arm-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+// RUN: %clang %s --target=armv7l-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARM %s
+
+/// armeb triples should be unmodified.
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabihf -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEBHF %s
+// RUN: %clang %s --target=armeb-unknown-linux-gnueabi -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=ARMEB %s
+
+// RUN: %clang %s --target=arm-pc-windows-msvc -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=WINDOWS %s
+/// armhf-pc... isn't recognised so just check that the float-abi option is ignored
+// RUN: %clang %s --target=arm-pc-windows-msvc -mfloat-abi=hard -print-runtime-dir \
+// RUN:-resource-dir=%S/Inputs/arm_float_abi_runtime_path 2>&1 | FileCheck -check-prefix=WINDOWS %s
+
+// ARMHF:   lib{{/|\\}}arm-unknown-linux-gnueabihf{{$}}
+// ARM: lib{{/|\\}}arm-unknown-linux-gnueabi{{$}}
+// ARMEBHF: lib{{/|\\}}a

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added reviewers: MaskRay, phosek.
DavidSpickett added a comment.
Herald added a subscriber: StephenFan.

The first attempt at this was https://reviews.llvm.org/D110142.

The key difference is that...

> It also seems like armhf-unknown-linux-gnueabihf duplicates the hf bit 
> (there's both armhf and gnueabihf) which seems unnecessary.

I have solved this. The folder name is no longer a redundant (and, I found out, 
invalid) triple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140011

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

[PATCH] D140011: [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD

[David Spickett via Phabricator via cfe-commits]

DavidSpickett created this revision.
Herald added subscribers: Enna1, kristof.beyls, dberris.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added subscribers: llvm-commits, Sanitizers, cfe-commits, MaskRay.
Herald added projects: clang, Sanitizers, LLVM.

The orginal single folder layout produced libraries in the form:
lib/linux/-.a

That archname for Arm depended on whether you had hard or soft float.
This is sometimes shown as "arm" (soft) vs. "armhf" (hard).

If this is set in a triple we do it in the final portion, the ABI.
"gnueabi" for soft float, "gnueabihf" for hard float.

Meaning that the expected triple is:
arm-unknown-linux-gnueabihf
Not this:
armhf-unknown-linux-gnueabihf

For the per target layout I have decided to rely on the ABI portion
of the triple instead of the arch name used for the old layout
(doing that would produce the invalid triple above).

This means that building with triple:
armv8l-unknown-linux-gnueabihf
Will result in libraries in:
lib/arm-unknown-linux-gnueabihf/

And clang will now know to look for "arm" instead of "armv8l".
Meaning that we can share libraries between an armv8 and armv7 build
as we did with the previous layout. In addition to handling spelling
differences e.g. "armv8l" with an "l" on some Linux distros.

compiler-rt will autodetect that the "armhf" and/or "arm" architecture
can be built. We then replace the given triple's architecture with that.
Then if the triple's float ABI doesn't match, we change that. That new
triple is then used as the folder name.

If you select to build only the given triple, with 
COMPILER_RT_DEFAULT_TARGET_ONLY,
compiler-rt will not autodetect the architecture and for that I assume you
know what you're doing. In that case the library path will use the unomdified 
triple.

>From what I can tell, this is how most large builds e.g Android and
Arm's Embedded Toolchain for llvm do things. I assume that big endian "armeb"
builds would end up doing this too.

Bare metal builds will not be using per target runtime dirs so they
remain as they were.

Depends on D139536 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140011

Files:
  clang/lib/Driver/ToolChain.cpp
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-pc-windows-msvc/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/arm-unknown-linux-gnueabihf/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabi/.keep
  
clang/test/Driver/Inputs/arm_float_abi_runtime_path/lib/armeb-unknown-linux-gnueabihf/.keep
  clang/test/Driver/arm-float-abi-runtime-path.c
  compiler-rt/cmake/Modules/CompilerRTUtils.cmake
  llvm/CMakeLists.txt

Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -816,7 +816,7 @@
 mark_as_advanced(LLVM_TARGET_TRIPLE_ENV)
 
 # Per target dir not yet supported on Arm 32 bit due to arm vs armhf handling
-if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux" AND NOT CMAKE_SYSTEM_PROCESSOR MATCHES "^arm")
+if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux")
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)
 else()
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF)
Index: compiler-rt/cmake/Modules/CompilerRTUtils.cmake
===
--- compiler-rt/cmake/Modules/CompilerRTUtils.cmake
+++ compiler-rt/cmake/Modules/CompilerRTUtils.cmake
@@ -433,6 +433,25 @@
 string(REGEX REPLACE "mipsisa64" "mipsisa32" triple_cpu_mips "${triple_cpu}")
 string(REGEX REPLACE "mips64" "mips" triple_cpu_mips "${triple_cpu_mips}")
 set(target "${triple_cpu_mips}${triple_suffix_gnu}")
+  elseif("${arch}" MATCHES "^arm")
+# Arch is arm, armhf, armv6m (anything else would come from using
+# COMPILER_RT_DEFAULT_TARGET_ONLY, which is checked above).
+if (${arch} STREQUAL "armhf")
+  # If we are building for hard float but our ABI is soft float.
+  if ("${triple_suffix}" MATCHES ".*eabi$")
+# Change "eabi" -> "eabihf"
+set(triple_suffix "${triple_suffix}hf")
+  endif()
+  # ABI is already set in the triple, don't repeat it in the architecture.
+  set(arch "arm")
+else ()
+  # If we are building for soft float, but the triple's ABI is hard float.
+  if ("${triple_suffix}" MATCHES ".*eabihf$")
+# Change "eabihf" -> "eabi"
+string(REGEX REPLACE "hf$" "" triple_suffix "${triple_suffix}")
+  endif()
+endif()
+set(target "${arch}${triple_suffix}")
   else()
 set(target "${arch}${triple_suffix}")
   endif()
Index: clang/test/Driver/arm-float-abi-runtime-path.c
===
--- /dev/null
+++ clang/test/Driver/arm-float-abi-runtime-path.c
@@ -0,0 +1,33 @@
+/// Check that libraries built 

[PATCH] D137836: [Support] Move getHostNumPhysicalCores to Threading.h

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Looks like tests need updating for that new -1 return value: 
https://lab.llvm.org/buildbot/#/builders/178/builds/3419

That bot builds with threading disabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137836

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

[PATCH] D138792: [AArch64] Improve TargetParser API

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

Looks like a good direction to me. Plenty more bits in clang that should really 
live in the target parser, and this is a great start at moving those.

My only issue is the use of `AI` for ArchInfo. I see that ArchKind was also a 
type and a var name. If this new name is following the codestyle then fine but 
I'd go with ArchInfo (`arch_info` but that's against the style, sigh). Up to 
you, I know the acronym style is common elsewhere and IDE helpers mitigate it 
somewhat.




Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:119
+  ArchInfo(const ArchInfo &) = delete;
+  ArchInfo &operator=(const ArchInfo &rhs) = delete;
+

Do you need to do anything to prevent moving from this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138792

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

[PATCH] D138157: Make -fsanitize=scudo use scudo_standalone. Delete check-scudo.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

We (Linaro) also have an issue with a bot that uses 
`-DCOMPILER_RT_BUILD_SANITIZERS=OFF`.

https://lab.llvm.org/buildbot/#/builders/178/builds/3318

  CMake Error at cmake/modules/AddLLVM.cmake:1915 (add_dependencies):
The dependency target "ScudoUnitTests" of target "check-all" does not
exist.
  Call Stack (most recent call first):
cmake/modules/AddLLVM.cmake:1955 (add_lit_target)
CMakeLists.txt:1249 (umbrella_lit_testsuite_end)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138157

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

[PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

lldb parts LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

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

[PATCH] D137338: Fix dupe word typos

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Looked at the lldb changes, some comments for you. If you want to get a "looks 
good" for those please submit a separate review with only the lldb parts and 
I'll review that instead.

As others said, appreciate the effort but the review process doesn't scale well 
to so many changes in one patch.




Comment at: lldb/include/lldb/Target/Process.h:1204
   /// platform that might itself be running natively, but have different
-  /// heuristics for figuring out which OS is is emulating.
+  /// heuristics for figuring out which OS is emulating.
   ///

This should be "which OS it is emulating".



Comment at: lldb/source/Symbol/LineTable.cpp:92
 // after the prologue.
-// Instead of it it is issuing a line table entry for the first instruction
+// Instead of it is issuing a line table entry for the first instruction
 // of the prologue and one for the first instruction after the prologue. If

This should be "Instead it is issuing".



Comment at: lldb/source/Target/RegisterContextUnwind.cpp:701
   // can have arbitrary number of frames with the same CFA, but more then 2 is
-  // very very unlikely)
 

This one is intended. It's not very scientific but hey, it's getting the point 
across.



Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:1045
   auto type_cstr = type_obj.GetDisplayTypeName();
-  // If we have a type with many many children, we would like to be able to
   // give a hint to the IDE that the type has indexed children so that the

This one is intended. Given the context of batching work, it's making the point 
for very large amounts of children.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137338

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

[PATCH] D135583: [LLVM] Use DWARFv4 bitfields when tuning for GDB

[David Spickett via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc6e2de60423a: [LLVM] Use DWARFv4 bitfields when tuning for 
GDB (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135583

Files:
  clang/docs/ReleaseNotes.rst
  llvm/docs/ReleaseNotes.rst
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
  llvm/test/DebugInfo/X86/packed_bitfields.ll


Index: llvm/test/DebugInfo/X86/packed_bitfields.ll
===
--- llvm/test/DebugInfo/X86/packed_bitfields.ll
+++ llvm/test/DebugInfo/X86/packed_bitfields.ll
@@ -1,7 +1,5 @@
 ; RUN: llc -dwarf-version=2 -mtriple x86_64-apple-macosx -O0 -filetype=obj -o 
%t_2_le.o %s
 ; RUN: llvm-dwarfdump -v -debug-info %t_2_le.o | FileCheck %s
-; RUN: llc -dwarf-version=4 -debugger-tune=gdb -mtriple x86_64-apple-macosx 
-O0 -filetype=obj -o %t_4_le.o %s
-; RUN: llvm-dwarfdump -v -debug-info %t_4_le.o | FileCheck %s
 
 ; Produced at -O0 from:
 ; struct {
Index: llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
===
--- llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
+++ llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
@@ -1,8 +1,9 @@
 ; RUN: llc -mtriple x86_64-apple-macosx -O0 -filetype=obj -o - %s \
 ; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ; RUN: llc -mtriple x86_64-gnu-linux -O0 -filetype=obj -o - %s \
-; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s --check-prefix=LINUX
-; LINUX-NOT: DW_AT_data_bit_offset
+; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
+; RUN: llc -mtriple x86_64-gnu-linux -O0 -debugger-tune=gdb -filetype=obj -o - 
%s \
+; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ;
 ; Generated from:
 ;   #include 
Index: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -428,8 +428,7 @@
   // https://sourceware.org/bugzilla/show_bug.cgi?id=11616
   UseGNUTLSOpcode = tuneForGDB() || DwarfVersion < 3;
 
-  // GDB does not fully support the DWARF 4 representation for bitfields.
-  UseDWARF2Bitfields = (DwarfVersion < 4) || tuneForGDB();
+  UseDWARF2Bitfields = DwarfVersion < 4;
 
   // The DWARF v5 string offsets table has - possibly shared - contributions
   // from each compile and type unit each preceded by a header. The string
Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -177,7 +177,12 @@
 Changes to the Debug Info
 -
 
-During this release ...
+Previously when emitting DWARF v4 and tuning for GDB, llc would use DWARF v2's
+``DW_AT_bit_offset`` and ``DW_AT_data_member_location``. llc now uses DWARF 
v4's
+``DW_AT_data_bit_offset`` regardless of tuning.
+
+Support for ``DW_AT_data_bit_offset`` was added in GDB 8.0. For earlier 
versions,
+you can use llc's ``-dwarf-version=3`` option to emit compatible DWARF.
 
 Changes to the LLVM tools
 -
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -624,6 +624,13 @@
 DWARF Support in Clang
 --
 
+Previously when emitting DWARFv4 and tuning for GDB, Clang would use DWARF v2's
+``DW_AT_bit_offset`` and ``DW_AT_data_member_location``. Clang now uses DWARF 
v4's
+``DW_AT_data_bit_offset`` regardless of tuning.
+
+Support for ``DW_AT_data_bit_offset`` was added in GDB 8.0. For earlier 
versions,
+you can use the ``-gdwarf-3`` option to emit compatible DWARF.
+
 Arm and AArch64 Support in Clang
 
 


Index: llvm/test/DebugInfo/X86/packed_bitfields.ll
===
--- llvm/test/DebugInfo/X86/packed_bitfields.ll
+++ llvm/test/DebugInfo/X86/packed_bitfields.ll
@@ -1,7 +1,5 @@
 ; RUN: llc -dwarf-version=2 -mtriple x86_64-apple-macosx -O0 -filetype=obj -o %t_2_le.o %s
 ; RUN: llvm-dwarfdump -v -debug-info %t_2_le.o | FileCheck %s
-; RUN: llc -dwarf-version=4 -debugger-tune=gdb -mtriple x86_64-apple-macosx -O0 -filetype=obj -o %t_4_le.o %s
-; RUN: llvm-dwarfdump -v -debug-info %t_4_le.o | FileCheck %s
 
 ; Produced at -O0 from:
 ; struct {
Index: llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
===
--- llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
+++ llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
@@ -1,8 +1,9 @@
 ; RUN: llc -mtriple x86_64-apple-macosx -O0 -filetype=obj -o - %s \
 ; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ; RUN: llc 

[PATCH] D135583: [LLVM] Use DWARFv4 bitfields when tuning for GDB

[David Spickett via Phabricator via cfe-commits]

DavidSpickett updated this revision to Diff 471065.
DavidSpickett added a comment.

- DWARFvN -> DWARF vN
- Remove superfluous brackets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135583

Files:
  clang/docs/ReleaseNotes.rst
  llvm/docs/ReleaseNotes.rst
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
  llvm/test/DebugInfo/X86/packed_bitfields.ll


Index: llvm/test/DebugInfo/X86/packed_bitfields.ll
===
--- llvm/test/DebugInfo/X86/packed_bitfields.ll
+++ llvm/test/DebugInfo/X86/packed_bitfields.ll
@@ -1,7 +1,5 @@
 ; RUN: llc -dwarf-version=2 -mtriple x86_64-apple-macosx -O0 -filetype=obj -o 
%t_2_le.o %s
 ; RUN: llvm-dwarfdump -v -debug-info %t_2_le.o | FileCheck %s
-; RUN: llc -dwarf-version=4 -debugger-tune=gdb -mtriple x86_64-apple-macosx 
-O0 -filetype=obj -o %t_4_le.o %s
-; RUN: llvm-dwarfdump -v -debug-info %t_4_le.o | FileCheck %s
 
 ; Produced at -O0 from:
 ; struct {
Index: llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
===
--- llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
+++ llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
@@ -1,8 +1,9 @@
 ; RUN: llc -mtriple x86_64-apple-macosx -O0 -filetype=obj -o - %s \
 ; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ; RUN: llc -mtriple x86_64-gnu-linux -O0 -filetype=obj -o - %s \
-; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s --check-prefix=LINUX
-; LINUX-NOT: DW_AT_data_bit_offset
+; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
+; RUN: llc -mtriple x86_64-gnu-linux -O0 -debugger-tune=gdb -filetype=obj -o - 
%s \
+; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ;
 ; Generated from:
 ;   #include 
Index: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -428,8 +428,7 @@
   // https://sourceware.org/bugzilla/show_bug.cgi?id=11616
   UseGNUTLSOpcode = tuneForGDB() || DwarfVersion < 3;
 
-  // GDB does not fully support the DWARF 4 representation for bitfields.
-  UseDWARF2Bitfields = (DwarfVersion < 4) || tuneForGDB();
+  UseDWARF2Bitfields = DwarfVersion < 4;
 
   // The DWARF v5 string offsets table has - possibly shared - contributions
   // from each compile and type unit each preceded by a header. The string
Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -177,7 +177,12 @@
 Changes to the Debug Info
 -
 
-During this release ...
+Previously when emitting DWARF v4 and tuning for GDB, llc would use DWARF v2's
+``DW_AT_bit_offset`` and ``DW_AT_data_member_location``. llc now uses DWARF 
v4's
+``DW_AT_data_bit_offset`` regardless of tuning.
+
+Support for ``DW_AT_data_bit_offset`` was added in GDB 8.0. For earlier 
versions,
+you can use llc's ``-dwarf-version=3`` option to emit compatible DWARF.
 
 Changes to the LLVM tools
 -
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -624,6 +624,13 @@
 DWARF Support in Clang
 --
 
+Previously when emitting DWARFv4 and tuning for GDB, Clang would use DWARF v2's
+``DW_AT_bit_offset`` and ``DW_AT_data_member_location``. Clang now uses DWARF 
v4's
+``DW_AT_data_bit_offset`` regardless of tuning.
+
+Support for ``DW_AT_data_bit_offset`` was added in GDB 8.0. For earlier 
versions,
+you can use the ``-gdwarf-3`` option to emit compatible DWARF.
+
 Arm and AArch64 Support in Clang
 
 


Index: llvm/test/DebugInfo/X86/packed_bitfields.ll
===
--- llvm/test/DebugInfo/X86/packed_bitfields.ll
+++ llvm/test/DebugInfo/X86/packed_bitfields.ll
@@ -1,7 +1,5 @@
 ; RUN: llc -dwarf-version=2 -mtriple x86_64-apple-macosx -O0 -filetype=obj -o %t_2_le.o %s
 ; RUN: llvm-dwarfdump -v -debug-info %t_2_le.o | FileCheck %s
-; RUN: llc -dwarf-version=4 -debugger-tune=gdb -mtriple x86_64-apple-macosx -O0 -filetype=obj -o %t_4_le.o %s
-; RUN: llvm-dwarfdump -v -debug-info %t_4_le.o | FileCheck %s
 
 ; Produced at -O0 from:
 ; struct {
Index: llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
===
--- llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
+++ llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
@@ -1,8 +1,9 @@
 ; RUN: llc -mtriple x86_64-apple-macosx -O0 -filetype=obj -o - %s \
 ; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ; RUN: llc -mtriple x86_64-gnu-linux -O0 -filetype=obj -o - %s \
-; RUN: | llvm-dwarfdump -v -debug-info - | FileCh

[PATCH] D135583: [LLVM] Use DWARFv4 bitfields when tuning for GDB

[David Spickett via Phabricator via cfe-commits]

DavidSpickett updated this revision to Diff 470766.
DavidSpickett added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Test that linux DWARFv4 uses data_bit_offset and that adding GDB tuning does 
not change that.
- Add release notes for clang and llvm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135583

Files:
  clang/docs/ReleaseNotes.rst
  llvm/docs/ReleaseNotes.rst
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
  llvm/test/DebugInfo/X86/packed_bitfields.ll


Index: llvm/test/DebugInfo/X86/packed_bitfields.ll
===
--- llvm/test/DebugInfo/X86/packed_bitfields.ll
+++ llvm/test/DebugInfo/X86/packed_bitfields.ll
@@ -1,7 +1,5 @@
 ; RUN: llc -dwarf-version=2 -mtriple x86_64-apple-macosx -O0 -filetype=obj -o 
%t_2_le.o %s
 ; RUN: llvm-dwarfdump -v -debug-info %t_2_le.o | FileCheck %s
-; RUN: llc -dwarf-version=4 -debugger-tune=gdb -mtriple x86_64-apple-macosx 
-O0 -filetype=obj -o %t_4_le.o %s
-; RUN: llvm-dwarfdump -v -debug-info %t_4_le.o | FileCheck %s
 
 ; Produced at -O0 from:
 ; struct {
Index: llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
===
--- llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
+++ llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
@@ -1,8 +1,9 @@
 ; RUN: llc -mtriple x86_64-apple-macosx -O0 -filetype=obj -o - %s \
 ; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ; RUN: llc -mtriple x86_64-gnu-linux -O0 -filetype=obj -o - %s \
-; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s --check-prefix=LINUX
-; LINUX-NOT: DW_AT_data_bit_offset
+; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
+; RUN: llc -mtriple x86_64-gnu-linux -O0 -debugger-tune=gdb -filetype=obj -o - 
%s \
+; RUN: | llvm-dwarfdump -v -debug-info - | FileCheck %s
 ;
 ; Generated from:
 ;   #include 
Index: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -428,8 +428,7 @@
   // https://sourceware.org/bugzilla/show_bug.cgi?id=11616
   UseGNUTLSOpcode = tuneForGDB() || DwarfVersion < 3;
 
-  // GDB does not fully support the DWARF 4 representation for bitfields.
-  UseDWARF2Bitfields = (DwarfVersion < 4) || tuneForGDB();
+  UseDWARF2Bitfields = (DwarfVersion < 4);
 
   // The DWARF v5 string offsets table has - possibly shared - contributions
   // from each compile and type unit each preceded by a header. The string
Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -177,7 +177,12 @@
 Changes to the Debug Info
 -
 
-During this release ...
+Previously when emitting DWARFv4 and tuning for GDB, llc would use DWARFv2's
+``DW_AT_bit_offset`` and ``DW_AT_data_member_location``. llc now uses DWARFv4's
+``DW_AT_data_bit_offset`` regardless of tuning.
+
+Support for ``DW_AT_data_bit_offset`` was added in GDB 8.0. For earlier 
versions,
+you can use llc's ``-dwarf-version=3`` option to emit compatible DWARF.
 
 Changes to the LLVM tools
 -
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -616,6 +616,13 @@
 DWARF Support in Clang
 --
 
+Previously when emitting DWARFv4 and tuning for GDB, Clang would use DWARFv2's
+``DW_AT_bit_offset`` and ``DW_AT_data_member_location``. Clang now uses 
DWARFv4's
+``DW_AT_data_bit_offset`` regardless of tuning.
+
+Support for ``DW_AT_data_bit_offset`` was added in GDB 8.0. For earlier 
versions,
+you can use the ``-gdwarf-3`` option to emit compatible DWARF.
+
 Arm and AArch64 Support in Clang
 
 


Index: llvm/test/DebugInfo/X86/packed_bitfields.ll
===
--- llvm/test/DebugInfo/X86/packed_bitfields.ll
+++ llvm/test/DebugInfo/X86/packed_bitfields.ll
@@ -1,7 +1,5 @@
 ; RUN: llc -dwarf-version=2 -mtriple x86_64-apple-macosx -O0 -filetype=obj -o %t_2_le.o %s
 ; RUN: llvm-dwarfdump -v -debug-info %t_2_le.o | FileCheck %s
-; RUN: llc -dwarf-version=4 -debugger-tune=gdb -mtriple x86_64-apple-macosx -O0 -filetype=obj -o %t_4_le.o %s
-; RUN: llvm-dwarfdump -v -debug-info %t_4_le.o | FileCheck %s
 
 ; Produced at -O0 from:
 ; struct {
Index: llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
===
--- llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
+++ llvm/test/DebugInfo/X86/bitfields-dwarf4.ll
@@ -1,8 +1,9 @@
 ; RUN: llc -mtriple x86_64-apple-macosx -O0 -filetype=obj -o - %s \
 ; RUN: | llvm-dwarfdu

[PATCH] D136124: [clang][deps] Remove unintentional `move`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

LGTM

As it happens the bot that found this has been moved to silent for other 
reasons. I'll let you know if we see any further issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136124

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

[PATCH] D136124: [clang][deps] Remove unintentional `move`

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:400
+auto OverlayFS =
+llvm::makeIntrusiveRefCnt(BaseFS);
 auto InMemoryFS =

Is this equivalent?
```
auto OverlayFS = BaseFS;
```
Given that BaseFS is already `IntrusiveRefCntPtr`.



Comment at: clang/test/ClangScanDeps/modules-full-by-mod-name.cpp:18
 //
-// RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 4 -format 
experimental-full \
+// RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -format 
experimental-full \
 // RUN:   -mode preprocess-dependency-directives -module-name=header1 > 
%t_clangcl.result

Would it help to have one be j1 and one j4, any extra coverage by doing that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136124

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

[PATCH] D135414: [clang][deps] Don't share in-memory VFS across scans

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Hi @jansvoboda11 , we (Linaro) happen to be one of the few running a bot with 
threading disabled (for reasons that aren't important here). It has been red a 
lot lately so you won't have seen the report from it.

https://lab.llvm.org/buildbot/#/builders/178/builds/3045

This does not fail on our other bots and I think `LLVM_ENABLE_THREADS=OFF` is 
what breaks it.

From a working build:

  $ /home/david.spickett/build-llvm-arm/bin/clang-scan-deps 
-compilation-database 
/home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb
 -j 4 -format experimental-full -mode preprocess-dependency-directives 
-module-name=header1
  Building DependencyScanningWorker
  Building DependencyScanningWorker
  Building DependencyScanningWorker
  Building DependencyScanningWorker
  BaseFS was not null
  BaseFS was not null
  BaseFS moved
  BaseFS moved

From a build with no threading:

  $ /home/david.spickett/build-llvm-arm-broken/bin/clang-scan-deps 
-compilation-database 
/home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb
 -j 4 -format experimental-full -mode preprocess-dependency-directives 
-module-name=header1
  Building DependencyScanningWorker
  BaseFS was not null
  BaseFS moved
  clang-scan-deps: 
/home/david.spickett/llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:393:
 bool 
clang::tooling::dependencies::DependencyScanningWorker::computeDependencies(llvm::StringRef,
 const std::vector &, 
clang::tooling::dependencies::DependencyConsumer &, clang::DiagnosticConsumer 
&, llvm::Optional): Assertion `BaseFS' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace.
  Stack dump:
  0.  Program arguments: 
/home/david.spickett/build-llvm-arm-broken/bin/clang-scan-deps 
-compilation-database 
/home/david.spickett/build-llvm-arm-broken/tools/clang/test/ClangScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb
 -j 4 -format experimental-full -mode preprocess-dependency-directives 
-module-name=header1
  Aborted (core dumped)

I think I narrowed it down to the move you added (I put a comment on it). When 
threading is enabled 4 workers are made and so no one tries to use `BaseFs` 
after it has been moved from. When only one worker is made, it attempts to use 
after move `BaseFs`.

In fact, if you give `-j 1` to the threaded version, it also crashes:

  $ /home/david.spickett/build-llvm-arm/bin/clang-scan-deps 
-compilation-database 
/home/david.spickett/build-llvm-arm-broken/tools/clang/test/Cla
  ngScanDeps/Output/modules-full-by-mod-name.cpp.tmp.cdb -j 1 -format 
experimental-full -mode preprocess-dependency-directives -module-name=header1
  Building DependencyScanningWorker
  BaseFS was not null
  BaseFS moved
  clang-scan-deps: 
/home/david.spickett/llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:393:
 bool 
clang::tooling::dependencies::DependencyScanningWorker::computeDependencies(llvm::StringRef,
 const std::vector &, 
clang::tooling::dependencies::DependencyConsumer &, clang::DiagnosticConsumer 
&, llvm::Optional): Assertion `BaseFS' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace.
  Aborted (core dumped)

I think the same sort of thing would happen if you said `-j N+1` when you had N 
cores? One worker would be run twice? I'm not up on all the details.

So yeah, to reproduce build without threading or add `-j 1`.

Do you have an idea how this might be handled? If you are moving you clearly 
want to transfer ownership, but I'm not sure if the `BaseFs` can just be 
rebuilt each time. I suppose if N workers can each have a `BaseFs`, a single 
worker could recreate it as needed.

Let me know what you think there.




Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:401
+auto OverlayFS = llvm::makeIntrusiveRefCnt(
+std::move(BaseFS));
+auto InMemoryFS =

This move seems like a problem when only one worker is made.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135414

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

[PATCH] D136070: [LIT] Add AArch64/Windows to LP64 feature

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136070

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

[PATCH] D135461: [LLDB] Fix crash when printing a struct with a static wchar_t member

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

> although those changes don't actually test the code path changed here

Yeah I'm just cargo culting on that one so it doesn't look strange that a few 
are missing.

If we're going to change the suffix that can be done elsewhere and reviewed by 
someone who understands them better than me :)

Not crashing is an improvement in itself, LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135461

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

[PATCH] D135461: [LLDB] Fix crash when printing a struct with a static wchar_t member

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

You need to update the python file for the test as well.

What does the output look like, do we actually print the character itself or 
it's value? (just curious, which one we do is probably out of scope for this 
change)




Comment at: clang/lib/AST/StmtPrinter.cpp:1298
+  case BuiltinType::WChar_U:
+break; // no suffix.
   }

Is there any reason to have a suffix here?

I admit this function is puzzling to me anyway given that char has a prefix and 
this won't. Perhaps this is because char here is not "character" it's an 
integer of a certain size. Where wide char is more about, well, being an actual 
character.

And reading https://en.cppreference.com/w/cpp/language/character_literal the 
suffix would be "L" which we've already used.

As long as it prints in a way that's useful for the developer that's fine.



Comment at: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp:38
   std::numeric_limits::max();
+  const static auto wchar_max = std::numeric_limits::max();
 

Is there a specific `signed wchar_t` and `unsigned wchar_t` like for char?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135461

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

[PATCH] D135170: [LLDB] Fix crash when printing a struct with a static signed char member

[David Spickett via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5a9e21305803: [LLDB] Fix crash when printing a struct with a 
static signed char member (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135170

Files:
  clang/lib/AST/StmtPrinter.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't 
crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_expr("const int *i = &A::int_val_with_address; *i",
  result_value="2")
 
+# Printing the whole type takes a slightly different code path. Check 
that
+# it does not crash.
+self.expect("image lookup -t A")
+
 # dsymutil strips the debug info for classes that only have const static
 # data members without a definition namespace scope.
 @expectedFailureAll(debug_info=["dsym"])
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -1280,6 +1280,7 @@
   case BuiltinType::Char_S:
   case BuiltinType::Char_U:OS << "i8"; break;
   case BuiltinType::UChar: OS << "Ui8"; break;
+  case BuiltinType::SChar: OS << "i8"; break;
   case BuiltinType::Short: OS << "i16"; break;
   case BuiltinType::UShort:OS << "Ui16"; break;
   case BuiltinType::Int:   break; // no suffix.


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_expr("const int *i = &A::int_val_with_address; *i",
  result_value="2")
 
+# Printing the whole type takes a slightly different code path. Check that
+# it does not crash.
+self.expect("image lookup -t A")
+
 # dsymutil strips the debug info for classes that only have const static
 # data members without a definition na

[PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[David Spickett via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG02c1c939486f: [LLDB] Fix printing a static bool struct 
member when using "image lookup -t" (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135169

Files:
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp

Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,8 +79,13 @@
   ScopedEnum::scoped_enum_case1;
 };
 
+struct StaticBoolStruct {
+  static const bool value = false;
+};
+
 int main() {
   A a;
+  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,6 +32,11 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
+# Test a bool member when printing the struct it is a member of.
+# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
+self.expect("image lookup -t StaticBoolStruct",
+substrs=["static const bool value = false;"])
+
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
Index: lldb/source/Symbol/CompilerType.cpp
===
--- lldb/source/Symbol/CompilerType.cpp
+++ lldb/source/Symbol/CompilerType.cpp
@@ -154,6 +154,12 @@
   return IsIntegerType(is_signed) || IsEnumerationType(is_signed);
 }
 
+bool CompilerType::IsBooleanType() const {
+  if (IsValid())
+return m_type_system->IsBooleanType(m_type);
+  return false;
+}
+
 bool CompilerType::IsPointerType(CompilerType *pointee_type) const {
   if (IsValid()) {
 return m_type_system->IsPointerType(m_type, pointee_type);
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -592,6 +592,8 @@
   bool IsEnumerationType(lldb::opaque_compiler_type_t type,
  bool &is_signed) override;
 
+  bool IsBooleanType(lldb::opaque_compiler_type_t type) override;
+
   bool IsScopedEnumerationType(lldb::opaque_compiler_type_t type) override;
 
   static bool IsObjCClassType(const CompilerType &type);
@@ -860,6 +862,8 @@
   static void SetIntegerInitializerForVariable(clang::VarDecl *var,
const llvm::APInt &init_value);
 
+  static void SetBoolInitializerForVariable(clang::VarDecl *var, bool value);
+
   /// Initializes a variable with a floating point value.
   /// \param var The variable to initialize. Must not already have an
   ///initializer and must have a floating point type.
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -3224,6 +3224,20 @@
   return false;
 }
 
+bool TypeSystemClang::IsBooleanType(lldb::opaque_compiler_type_t type) {
+  if (!type)
+return false;
+
+  clang::QualType qual_type(GetCanonicalQualType(type));
+  const clang::BuiltinType *builtin_type =
+  llvm::dyn_cast(qual_type->getCanonicalTypeInternal());
+
+  if (!builtin_type)
+return false;
+
+  return builtin_type->isBooleanType();
+}
+
 bool TypeSystemClang::IsEnumerationType(lldb::opaque_compiler_type_t type,
 bool &is_signed) {
   if (type) {
@@ -7574,6 +7588,18 @@
   return var_decl;
 }
 
+void TypeSystemClang::SetBoolInitializerForVariable(VarDecl *var, bool value) {
+  assert(!var->hasInit() && "variable already initialized");
+
+  QualType qt = var->getType();
+  assert(qt->isSpecificBuiltinType(BuiltinType::Bool

[PATCH] D134788: [ARM64EC][clang-cl] Add arm64EC test; NFC

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

This (and it's followup?) has been landed, right? Please close the revision if 
so.


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

https://reviews.llvm.org/D134788

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

[PATCH] D135256: [clang] Add Create method for CXXBoolLiteralExpr

[David Spickett via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7448f38898a8: [clang] Add Create method for 
CXXBoolLiteralExpr (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135256

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/Sema/SemaTemplate.cpp


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -7829,8 +7829,8 @@
 E = new (Context) CharacterLiteral(Arg.getAsIntegral().getZExtValue(),
Kind, T, Loc);
   } else if (T->isBooleanType()) {
-E = new (Context) CXXBoolLiteralExpr(Arg.getAsIntegral().getBoolValue(),
- T, Loc);
+E = CXXBoolLiteralExpr::Create(Context, Arg.getAsIntegral().getBoolValue(),
+   T, Loc);
   } else if (T->isNullPtrType()) {
 E = new (Context) CXXNullPtrLiteralExpr(Context.NullPtrTy, Loc);
   } else {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -7922,8 +7922,8 @@
   if (!ToLocationOrErr)
 return ToLocationOrErr.takeError();
 
-  return new (Importer.getToContext()) CXXBoolLiteralExpr(
-  E->getValue(), *ToTypeOrErr, *ToLocationOrErr);
+  return CXXBoolLiteralExpr::Create(Importer.getToContext(), E->getValue(),
+*ToTypeOrErr, *ToLocationOrErr);
 }
 
 ExpectedStmt ASTNodeImporter::VisitMemberExpr(MemberExpr *E) {
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -730,6 +730,11 @@
   explicit CXXBoolLiteralExpr(EmptyShell Empty)
   : Expr(CXXBoolLiteralExprClass, Empty) {}
 
+  static CXXBoolLiteralExpr *Create(const ASTContext &C, bool Val, QualType Ty,
+SourceLocation Loc) {
+return new (C) CXXBoolLiteralExpr(Val, Ty, Loc);
+  }
+
   bool getValue() const { return CXXBoolLiteralExprBits.Value; }
   void setValue(bool V) { CXXBoolLiteralExprBits.Value = V; }
 


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -7829,8 +7829,8 @@
 E = new (Context) CharacterLiteral(Arg.getAsIntegral().getZExtValue(),
Kind, T, Loc);
   } else if (T->isBooleanType()) {
-E = new (Context) CXXBoolLiteralExpr(Arg.getAsIntegral().getBoolValue(),
- T, Loc);
+E = CXXBoolLiteralExpr::Create(Context, Arg.getAsIntegral().getBoolValue(),
+   T, Loc);
   } else if (T->isNullPtrType()) {
 E = new (Context) CXXNullPtrLiteralExpr(Context.NullPtrTy, Loc);
   } else {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -7922,8 +7922,8 @@
   if (!ToLocationOrErr)
 return ToLocationOrErr.takeError();
 
-  return new (Importer.getToContext()) CXXBoolLiteralExpr(
-  E->getValue(), *ToTypeOrErr, *ToLocationOrErr);
+  return CXXBoolLiteralExpr::Create(Importer.getToContext(), E->getValue(),
+*ToTypeOrErr, *ToLocationOrErr);
 }
 
 ExpectedStmt ASTNodeImporter::VisitMemberExpr(MemberExpr *E) {
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -730,6 +730,11 @@
   explicit CXXBoolLiteralExpr(EmptyShell Empty)
   : Expr(CXXBoolLiteralExprClass, Empty) {}
 
+  static CXXBoolLiteralExpr *Create(const ASTContext &C, bool Val, QualType Ty,
+SourceLocation Loc) {
+return new (C) CXXBoolLiteralExpr(Val, Ty, Loc);
+  }
+
   bool getValue() const { return CXXBoolLiteralExprBits.Value; }
   void setValue(bool V) { CXXBoolLiteralExprBits.Value = V; }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135170: [LLDB] Fix crash when printing a struct with a static signed char member

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.



Comment at: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp:39
 
   const static auto char_min = std::numeric_limits::min();
   const static auto uchar_min = std::numeric_limits::min();

shafik wrote:
> We use `signed char` for the max case but `char` for the min case. Maybe we 
> need a max using `char` and a min using `signed char`.
I'll look into this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135170

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

[PATCH] D135170: [LLDB] Fix crash when printing a struct with a static signed char member

[David Spickett via Phabricator via cfe-commits]

DavidSpickett updated this revision to Diff 465342.
DavidSpickett added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135170

Files:
  clang/lib/AST/StmtPrinter.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't 
crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_expr("const int *i = &A::int_val_with_address; *i",
  result_value="2")
 
+# Printing the whole type takes a slightly different code path. Check 
that
+# it does not crash.
+self.expect("image lookup -t A")
+
 # dsymutil strips the debug info for classes that only have const static
 # data members without a definition namespace scope.
 @expectedFailureAll(debug_info=["dsym"])
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -1280,6 +1280,7 @@
   case BuiltinType::Char_S:
   case BuiltinType::Char_U:OS << "i8"; break;
   case BuiltinType::UChar: OS << "Ui8"; break;
+  case BuiltinType::SChar: OS << "i8"; break;
   case BuiltinType::Short: OS << "i16"; break;
   case BuiltinType::UShort:OS << "Ui16"; break;
   case BuiltinType::Int:   break; // no suffix.


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_expr("const int *i = &A::int_val_with_address; *i",
  result_value="2")
 
+# Printing the whole type takes a slightly different code path. Check that
+# it does not crash.
+self.expect("image lookup -t A")
+
 # dsymutil strips the debug info for classes that only have const static
 # data members without a definition namespace scope.
 @expectedFailureAll(debug_info=["dsym"])
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clan

[PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[David Spickett via Phabricator via cfe-commits]

DavidSpickett updated this revision to Diff 465341.
DavidSpickett added a comment.

Rebase, Create moved to parent change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135169

Files:
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp

Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,8 +79,13 @@
   ScopedEnum::scoped_enum_case1;
 };
 
+struct StaticBoolStruct {
+  static const bool value = false;
+};
+
 int main() {
   A a;
+  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,6 +32,11 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
+# Test a bool member when printing the struct it is a member of.
+# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
+self.expect("image lookup -t StaticBoolStruct",
+substrs=["static const bool value = false;"])
+
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
Index: lldb/source/Symbol/CompilerType.cpp
===
--- lldb/source/Symbol/CompilerType.cpp
+++ lldb/source/Symbol/CompilerType.cpp
@@ -154,6 +154,12 @@
   return IsIntegerType(is_signed) || IsEnumerationType(is_signed);
 }
 
+bool CompilerType::IsBooleanType() const {
+  if (IsValid())
+return m_type_system->IsBooleanType(m_type);
+  return false;
+}
+
 bool CompilerType::IsPointerType(CompilerType *pointee_type) const {
   if (IsValid()) {
 return m_type_system->IsPointerType(m_type, pointee_type);
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -592,6 +592,8 @@
   bool IsEnumerationType(lldb::opaque_compiler_type_t type,
  bool &is_signed) override;
 
+  bool IsBooleanType(lldb::opaque_compiler_type_t type) override;
+
   bool IsScopedEnumerationType(lldb::opaque_compiler_type_t type) override;
 
   static bool IsObjCClassType(const CompilerType &type);
@@ -860,6 +862,8 @@
   static void SetIntegerInitializerForVariable(clang::VarDecl *var,
const llvm::APInt &init_value);
 
+  static void SetBoolInitializerForVariable(clang::VarDecl *var, bool value);
+
   /// Initializes a variable with a floating point value.
   /// \param var The variable to initialize. Must not already have an
   ///initializer and must have a floating point type.
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -3224,6 +3224,20 @@
   return false;
 }
 
+bool TypeSystemClang::IsBooleanType(lldb::opaque_compiler_type_t type) {
+  if (!type)
+return false;
+
+  clang::QualType qual_type(GetCanonicalQualType(type));
+  const clang::BuiltinType *builtin_type =
+  llvm::dyn_cast(qual_type->getCanonicalTypeInternal());
+
+  if (!builtin_type)
+return false;
+
+  return builtin_type->isBooleanType();
+}
+
 bool TypeSystemClang::IsEnumerationType(lldb::opaque_compiler_type_t type,
 bool &is_signed) {
   if (type) {
@@ -7574,6 +7588,18 @@
   return var_decl;
 }
 
+void TypeSystemClang::SetBoolInitializerForVariable(VarDecl *var, bool value) {
+  assert(!var->hasInit() && "variable already initialized");
+
+  QualType qt = var->getType();
+  assert(qt->isSpecificBuiltinType(BuiltinType::Bool) &&
+ "only boolean supported");
+
+  clang::ASTContext &ast = var->getASTContext

[PATCH] D135256: [clang] Add Create method for CXXBoolLiteralExpr

[David Spickett via Phabricator via cfe-commits]

DavidSpickett created this revision.
Herald added a subscriber: martong.
Herald added a reviewer: shafik.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135256

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/Sema/SemaTemplate.cpp


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -7829,8 +7829,8 @@
 E = new (Context) CharacterLiteral(Arg.getAsIntegral().getZExtValue(),
Kind, T, Loc);
   } else if (T->isBooleanType()) {
-E = new (Context) CXXBoolLiteralExpr(Arg.getAsIntegral().getBoolValue(),
- T, Loc);
+E = CXXBoolLiteralExpr::Create(Context, Arg.getAsIntegral().getBoolValue(),
+   T, Loc);
   } else if (T->isNullPtrType()) {
 E = new (Context) CXXNullPtrLiteralExpr(Context.NullPtrTy, Loc);
   } else {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -7922,8 +7922,8 @@
   if (!ToLocationOrErr)
 return ToLocationOrErr.takeError();
 
-  return new (Importer.getToContext()) CXXBoolLiteralExpr(
-  E->getValue(), *ToTypeOrErr, *ToLocationOrErr);
+  return CXXBoolLiteralExpr::Create(Importer.getToContext(), E->getValue(),
+*ToTypeOrErr, *ToLocationOrErr);
 }
 
 ExpectedStmt ASTNodeImporter::VisitMemberExpr(MemberExpr *E) {
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -730,6 +730,11 @@
   explicit CXXBoolLiteralExpr(EmptyShell Empty)
   : Expr(CXXBoolLiteralExprClass, Empty) {}
 
+  static CXXBoolLiteralExpr *Create(const ASTContext &C, bool Val, QualType Ty,
+SourceLocation Loc) {
+return new (C) CXXBoolLiteralExpr(Val, Ty, Loc);
+  }
+
   bool getValue() const { return CXXBoolLiteralExprBits.Value; }
   void setValue(bool V) { CXXBoolLiteralExprBits.Value = V; }
 


Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -7829,8 +7829,8 @@
 E = new (Context) CharacterLiteral(Arg.getAsIntegral().getZExtValue(),
Kind, T, Loc);
   } else if (T->isBooleanType()) {
-E = new (Context) CXXBoolLiteralExpr(Arg.getAsIntegral().getBoolValue(),
- T, Loc);
+E = CXXBoolLiteralExpr::Create(Context, Arg.getAsIntegral().getBoolValue(),
+   T, Loc);
   } else if (T->isNullPtrType()) {
 E = new (Context) CXXNullPtrLiteralExpr(Context.NullPtrTy, Loc);
   } else {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -7922,8 +7922,8 @@
   if (!ToLocationOrErr)
 return ToLocationOrErr.takeError();
 
-  return new (Importer.getToContext()) CXXBoolLiteralExpr(
-  E->getValue(), *ToTypeOrErr, *ToLocationOrErr);
+  return CXXBoolLiteralExpr::Create(Importer.getToContext(), E->getValue(),
+*ToTypeOrErr, *ToLocationOrErr);
 }
 
 ExpectedStmt ASTNodeImporter::VisitMemberExpr(MemberExpr *E) {
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -730,6 +730,11 @@
   explicit CXXBoolLiteralExpr(EmptyShell Empty)
   : Expr(CXXBoolLiteralExprClass, Empty) {}
 
+  static CXXBoolLiteralExpr *Create(const ASTContext &C, bool Val, QualType Ty,
+SourceLocation Loc) {
+return new (C) CXXBoolLiteralExpr(Val, Ty, Loc);
+  }
+
   bool getValue() const { return CXXBoolLiteralExprBits.Value; }
   void setValue(bool V) { CXXBoolLiteralExprBits.Value = V; }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.



Comment at: clang/include/clang/AST/ExprCXX.h:733
 
+  static CXXBoolLiteralExpr *Create(const ASTContext &C, bool Val, QualType Ty,
+SourceLocation Loc) {

shafik wrote:
> I think this makes sense but if we are going to do this refactor then we 
> should clean up all the locations we are currently doing `new (Context) 
> CXXBoolLiteralExpr()` as well.
> 
> Maybe that should be the initial PR and then this change would be a follow-up.
Sounds good to me, I'll do that.



Comment at: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py:36
+# Test a bool member when printing the struct it is a member of.
+# TODO: replace this with printing struct A, once doing so doesn't 
crash lldb.
+self.expect("image lookup -t StaticBoolStruct",

And just to be clear, this is TODO is resolved in the patch stacked on this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135169

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

[PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.
Herald added a subscriber: JDevlieghere.

For https://github.com/llvm/llvm-project/issues/58135.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135169

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

[PATCH] D135170: [LLDB] Fix crash when printing a struct with a static signed char member

[David Spickett via Phabricator via cfe-commits]

DavidSpickett created this revision.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

As with static bool for whatever reason printing them on their own
worked fine but wasn't handled when you printed the whole type.

I don't see a good way to test this from clang's side so our existing
tests will have to do.

We can now print all of the struct "A", so there's no need for a separate
one for static bool testing. I've not checked the output, just that it
succeeds. This saves us having to handle different min/max between systems.

Depends on D135169 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135170

Files:
  clang/lib/AST/StmtPrinter.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't 
crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_expr("const int *i = &A::int_val_with_address; *i",
  result_value="2")
 
+# Printing the whole type takes a slightly different code path. Check 
that
+# it does not crash.
+self.expect("image lookup -t A")
+
 # dsymutil strips the debug info for classes that only have const static
 # data members without a definition namespace scope.
 @expectedFailureAll(debug_info=["dsym"])
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -1280,6 +1280,7 @@
   case BuiltinType::Char_S:
   case BuiltinType::Char_U:OS << "i8"; break;
   case BuiltinType::UChar: OS << "Ui8"; break;
+  case BuiltinType::SChar: OS << "i8"; break;
   case BuiltinType::Short: OS << "i16"; break;
   case BuiltinType::UShort:OS << "Ui16"; break;
   case BuiltinType::Int:   break; // no suffix.


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,13 +79,8 @@
   ScopedEnum::scoped_enum_case1;
 };
 
-struct StaticBoolStruct {
-  static const bool value = false;
-};
-
 int main() {
   A a;
-  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,11 +32,6 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
-# Test a bool member when printing the struct it is a member of.
-# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
-self.expect("image lookup -t StaticBoolStruct",
-substrs=["static const bool value = false;"])
-
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
@@ -88,6 +83,10 @@
 self.expect_ex

[PATCH] D135169: [LLDB] Fix printing a static bool struct member when using "image lookup -t"

[David Spickett via Phabricator via cfe-commits]

DavidSpickett created this revision.
Herald added a reviewer: shafik.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

Fixes #58135

Somehow lldb was able to print the member on its own but when we try
to print the whole type found by "image lookup -t" lldb would crash.

This is because we'd encoded the initial value of the member as an integer.
Which isn't the end of the world because bool is integral for C++.
However, clang has a special AST node to handle literal bool and it
expected us to use that instead.

This adds a new codepath to handle static bool which uses cxxBoolLiteralExpr
and we get the member printed as you'd expect.

For testing I added a struct with just the bool because trying to print
all of "A" crashes as well. Presumably because one of the other member's
types isn't handled properly either.

So for now I just added the bool case, we can merge it with A later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135169

Files:
  clang/include/clang/AST/ExprCXX.h
  lldb/include/lldb/Symbol/CompilerType.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerType.cpp
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp

Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -79,8 +79,13 @@
   ScopedEnum::scoped_enum_case1;
 };
 
+struct StaticBoolStruct {
+  static const bool value = false;
+};
+
 int main() {
   A a;
+  StaticBoolStruct sbs;
 
   auto char_max = A::char_max;
   auto uchar_max = A::uchar_max;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -32,6 +32,11 @@
 # Test a bool member.
 self.expect_expr("A::bool_val", result_value="true")
 
+# Test a bool member when printing the struct it is a member of.
+# TODO: replace this with printing struct A, once doing so doesn't crash lldb.
+self.expect("image lookup -t StaticBoolStruct",
+substrs=["static const bool value = false;"])
+
 # Test that minimum and maximum values for each data type are right.
 self.expect_expr("A::char_max == char_max", result_value="true")
 self.expect_expr("A::uchar_max == uchar_max", result_value="true")
Index: lldb/source/Symbol/CompilerType.cpp
===
--- lldb/source/Symbol/CompilerType.cpp
+++ lldb/source/Symbol/CompilerType.cpp
@@ -154,6 +154,12 @@
   return IsIntegerType(is_signed) || IsEnumerationType(is_signed);
 }
 
+bool CompilerType::IsBooleanType() const {
+  if (IsValid())
+return m_type_system->IsBooleanType(m_type);
+  return false;
+}
+
 bool CompilerType::IsPointerType(CompilerType *pointee_type) const {
   if (IsValid()) {
 return m_type_system->IsPointerType(m_type, pointee_type);
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -592,6 +592,8 @@
   bool IsEnumerationType(lldb::opaque_compiler_type_t type,
  bool &is_signed) override;
 
+  bool IsBooleanType(lldb::opaque_compiler_type_t type) override;
+
   bool IsScopedEnumerationType(lldb::opaque_compiler_type_t type) override;
 
   static bool IsObjCClassType(const CompilerType &type);
@@ -860,6 +862,8 @@
   static void SetIntegerInitializerForVariable(clang::VarDecl *var,
const llvm::APInt &init_value);
 
+  static void SetBoolInitializerForVariable(clang::VarDecl *var, bool value);
+
   /// Initializes a variable with a floating point value.
   /// \param var The variable to initialize. Must not already have an
   ///initializer and must have a floating point type.
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -3224,6 +3224,20 @@
   return false;
 }
 
+bool TypeSystemClang::Is

[PATCH] D134788: [ARM64EC][clang-cl] Add /arm64EC flag

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

This looks fine as is, all comments addressed as far as I can see.




Comment at: clang/lib/Driver/Driver.cpp:1384
+  TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+if (UArgs->hasArg(options::OPT__SLASH_arm64EC)) {
+  getDiags().Report(clang::diag::warn_target_override_arm64ec)

bcl5980 wrote:
> DavidSpickett wrote:
> > DavidSpickett wrote:
> > > This would read better to me if you put them all in one and put the most 
> > > important check first like:
> > > ```
> > > if ( UArgs->hasArg(options::OPT__SLASH_arm64EC) &&
> > > ( (TC.getTriple().getArch() != llvm::Triple::aarch64 ||
> > >   TC.getTriple().getSubArch() != 
> > > llvm::Triple::AArch64SubArch_arm64ec)) {
> > > ```
> > This is good but I didn't emphasise one bit. Putting the arm64ec option 
> > check first saves reading the rest if the reader knows it's not relevant.
> I believe the condition `UArgs->hasArg(options::OPT__SLASH_arm64EC)` should 
> be much heavier than 
> `(TC.getTriple().getArch() != llvm::Triple::aarch64 || 
> TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec)`.
> So I put the Arch and SubArch check first here. I don't understand why we 
> should put the hasArg check first.
If the cost of the check is higher then this is fine keep it as is.

The reason that I personally would reorder them (if costs were equal) is this.

Imagine this didn't have 3 conditions it had 100.
```
if ((A or B or C or D or E...
 
) && some_option_used)
```

Why did I, the reader, have to parse 99 seemingly random conditions (which have 
to live in my short term memory) only to find that oh, this is all predicated 
on that one option being passed. If I don't actually care about that one 
option, I just wasted my time.

And ok, the average if doesn't have 100 conditions but if you keep this in mind 
even for smaller checks it adds up over time.

Code is going to be read more than it is written.

This applies to early returns too. Imagine I care about the !=10 case:
```
void foo(int i) {
  if (i == 10) {
// 500 lines of code you don't care about 50% of the time
  } else {
// what you were actually looking for
return;
  }
```

That situation does happen a lot, and the same principle can be applied.

```
void foo(int i) {
  if (i != 10)
 return;

  // 500 lines of code you can read if you actually need to
}
```


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

https://reviews.llvm.org/D134788

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

[PATCH] D134788: [ARM64EC][clang-cl] Add /arm64EC flag

[David Spickett via Phabricator via cfe-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

Please mark comments as done if you feel that they have been addressed. (I know 
this can be a bit weird, do you mark them or do I, I go with the former, I can 
disagree if needed)

Lack of docs right now is fine, we have enough to know this exists and it's 
quite simple.

With my one final comment addressed, this LGTM.




Comment at: clang/lib/Driver/Driver.cpp:1384
+  TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+if (UArgs->hasArg(options::OPT__SLASH_arm64EC)) {
+  getDiags().Report(clang::diag::warn_target_override_arm64ec)

DavidSpickett wrote:
> This would read better to me if you put them all in one and put the most 
> important check first like:
> ```
> if ( UArgs->hasArg(options::OPT__SLASH_arm64EC) &&
> ( (TC.getTriple().getArch() != llvm::Triple::aarch64 ||
>   TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec)) {
> ```
This is good but I didn't emphasise one bit. Putting the arm64ec option check 
first saves reading the rest if the reader knows it's not relevant.


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

https://reviews.llvm.org/D134788

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

[PATCH] D134788: [ARM64EC][clang-cl] Add /arm64EC flag

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

Has Microsoft documented this option? Not doubting it exists but I mostly see 
how to use it rather than a specific page describing the option and its 
behaviour.

Not a big deal but if there is one, please include a link to it in the commit 
message.




Comment at: clang/lib/Driver/Driver.cpp:1384
+  TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+if (UArgs->hasArg(options::OPT__SLASH_arm64EC)) {
+  getDiags().Report(clang::diag::warn_target_override_arm64ec)

This would read better to me if you put them all in one and put the most 
important check first like:
```
if ( UArgs->hasArg(options::OPT__SLASH_arm64EC) &&
( (TC.getTriple().getArch() != llvm::Triple::aarch64 ||
  TC.getTriple().getSubArch() != llvm::Triple::AArch64SubArch_arm64ec)) {
```



Comment at: clang/test/Driver/cl-options.c:787
+// RUN: %clang_cl -vctoolsdir "" /arm64EC /c -### -- %s 2>&1 | FileCheck %s 
--check-prefix ARM64EC
+// ARM64EC: "-triple" "arm64ec-pc-windows-msvc19.20.0"
+

Add an `ARM64EC-NOT` check for this part to check there is no warning.

Not going to catch much in this change but if someone goes digging into target 
setting they might change it by other means than `--target` and not realise.



Comment at: clang/test/Driver/cl-options.c:790
+// RUN: %clang_cl -vctoolsdir "" /arm64EC /c -target x86_64-pc-windows-msvc  
-### -- %s 2>&1 | FileCheck %s --check-prefix ARM64EC_OVERRIDE
+// ARM64EC_OVERRIDE: warning: /arm64EC has been overridden by specified 
target:x86_64-pc-windows-msvc; option ignored
+

A space after the ':' is a bit easier to parse IMO (unless this is matching an 
msvc warning in which case fair enough keep it as is).


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

https://reviews.llvm.org/D134788

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

[PATCH] D134637: clang-tblgen build: avoid duplicate inclusion of libLLVMSupport

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

+1 from me also, but someone else should check that this is a reasonable way to 
implement it cmake wise (not that this is a horrible hack but I never can tell 
with cmake).

One more question, does this same issue potentially apply to llvm-tblgen and 
has that got any special cmake changes to account for it? (if it doesn't, leave 
it as is, but as a comparison point)




Comment at: clang/lib/Support/CMakeLists.txt:21
+  # libLLVM-*.so, to be used by clang-tblgen. This is so clang-tblgen doesn't
+  # accidentally link against libLLVMSupport twice (once statically and once 
via
+  # libLLVM-*.so).

nhaehnle wrote:
> DavidSpickett wrote:
> > Without this change, is it the case that it will always link against 
> > libLLVMSupport twice with the DYLIB conifg?
> > 
> > "accidentally" sounds like you could stumble into it but from what I see, 
> > it's always been doing this and once your other change lands, it would 
> > always result in an error.
> > ```
> > This is so clang-tblgen doesn't link against libLLVMSupport twice (once 
> > statically and once via libLLVM-*.so).
> > ```
> I meant "accidentally" in the sense that *-tblgen isn't supposed to link 
> against libLLVM-*.so, but ended up doing so after clangSupport was added 
> earlier this year. Perhaps I should just remover the adverb?
That would work, otherwise it seems like a thing that sometimes happens under 
conditions that aren't explained.



Comment at: clang/lib/Support/CMakeLists.txt:26
+DISABLE_LLVM_LINK_LLVM_DYLIB
+${clangSupport_sources})
+endif()

nhaehnle wrote:
> DavidSpickett wrote:
> > Can you detail which targets link to/include what and how the problem 
> > happens? I'm trying to understand why we can't just use 
> > `DISABLE_LLVM_LINK_LLVM_DYLIB` on its own here.
> clangSupport is included by clang-tblgen but also by libclangcpp. The 
> underlying idea is that of all the users of clangSupport, clang-tblgen is 
> special because it uses the DISABLE_LLVM_LINK_LLVM_DYLIB override.
> 
> clangSupport links against Support, which becomes a link against libLLVM-*.so 
> with LLVM_LINK_LLVM_DYLIB=ON. So, in an LLVM_LINK_LLVM_DYLIB=ON build, we get 
> with this change:
> 
> - clangSupport links against Support, which becomes dynamically linking 
> against libLLVM-*.so (this is unchanged)
> - clangSupport_tablegen links against Support statically
> - clang-tblgen links against clangSupport_tablegen (and also directly against 
> Support) statically
> - other users of clangSupport link against clangSupport somehow, and then 
> transitively dynamically against libLLVM-*.so
> 
> Does that answer your questions?
> 
> Specifically, if we were to just add DISABLE_LLVM_LINK_LLVM_DYLIB to 
> clangSupport, then we risk a situation where some other user of clangSupport 
> links against Support twice; once via the copy of Support that is statically 
> linked to from clangSupport; and once via libLLVM-*.so that gets pulled in 
> via other dependencies. To be honest, I don't know for certain whether that 
> is a problem that would happen, but it seemed likely enough to me that I 
> wouldn't want to risk it.
> Does that answer your questions?

Yes but I don't think I have the expertise to say this is a good way to 
implement this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134637

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

[PATCH] D134637: clang-tblgen build: avoid duplicate inclusion of libLLVMSupport

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.



Comment at: clang/lib/Support/CMakeLists.txt:21
+  # libLLVM-*.so, to be used by clang-tblgen. This is so clang-tblgen doesn't
+  # accidentally link against libLLVMSupport twice (once statically and once 
via
+  # libLLVM-*.so).

Without this change, is it the case that it will always link against 
libLLVMSupport twice with the DYLIB conifg?

"accidentally" sounds like you could stumble into it but from what I see, it's 
always been doing this and once your other change lands, it would always result 
in an error.
```
This is so clang-tblgen doesn't link against libLLVMSupport twice (once 
statically and once via libLLVM-*.so).
```



Comment at: clang/lib/Support/CMakeLists.txt:26
+DISABLE_LLVM_LINK_LLVM_DYLIB
+${clangSupport_sources})
+endif()

Can you detail which targets link to/include what and how the problem happens? 
I'm trying to understand why we can't just use `DISABLE_LLVM_LINK_LLVM_DYLIB` 
on its own here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134637

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

[PATCH] D133711: [Sema] Reject array element types whose sizes aren't a multiple of their alignments

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

GCC also rejects those tests so https://reviews.llvm.org/D134417 to disable 
them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133711

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

[PATCH] D133711: [Sema] Reject array element types whose sizes aren't a multiple of their alignments

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

This has caused 2 test suite failures across our bots, for example: 
https://lab.llvm.org/buildbot/#/builders/174/builds/13518 
`GCC-C-execute-pr36093.test` / `GCC-C-execute-pr43783.test`

Pretty likely the tests are doing something strange, I will look into it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133711

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

[PATCH] D125419: [Arm64EC 7/?] clang side of Arm64EC varargs ABI.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I don't have the expertise to approve, but one question.

Why is there more testing in the `.c` than in `.cpp`. Is the same logic being 
used for both so there's no point checking twice?

If so what things is the `.cpp` test looking for specifically? I struggle to 
see how the lines it checks for map into the call to the variadic function. Is 
it looking at the mangling, the choice of pointer or not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125419

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

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added subscribers: nickdesaulniers, psmith.
DavidSpickett added a comment.

> 1:38 AM  might be good to take them out here as well

So I looked into this. Here are the Arm architectures that clang has that
gcc doesn't:
"armv5tej" // Not in GCC, j = jazelle
"armv7k" // Apple Watch S1
"armv7s" // iPhone 5
"xscale"

(minus some very new ones that are just because I used trunk clang)

These are the gcc architectures that clang doesn't have:
"armv6j"
"armv6s-m"
"armv6z"
"armv6zk"
"armv7"

So a random grab bag of v6 and 7 on both sides. Not worth disturbing that
now.

Arnd picked out jazelle. GCC doesn't list armv5tej but it does accept it
https://godbolt.org/z/cazcbfjGY. For armv6j clang doesn't list it but it
also does accept it https://godbolt.org/z/aEKfnojTY.

llvm's support for jazelle is a single instruction "bxj" which is a branch
exchange, but for jazelle (
https://developer.arm.com/documentation/dui0473/j/arm-and-thumb-instructions/bxj
).
Which makes some sense. If you had that one instruction you could compile
99% of the code with public toolchains, then the rest with some tool from
Arm, I guess. GCC appears to have done the same.

So targets wise I think things are fine as is unless a v6 enthusiast tries
to use clang sometime.

For jazelle the bxj instruction appears to be present on non jazelle
processors, and can be used as a normal bx in ThumbEE. The code to
implement it and the armv5tej target is minimal and compact so I think we
keep it for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133109

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

For (1) and (2) is there a need to be able to reset the architecture setting no 
matter the previous `march` and `` are?

Currently we're talking about *not* wanting having to use `-march=armv8-a+crc` 
but would you still want a way to reset the architecture no matter what the 
previous options are?

That said, applying the extra extensions to the last `-march` gives you a way 
to bump the base architecture which could be useful. `-march=armv8-a 
-add-extension=+crc -march=armv8.4-a` => armv8.4-a+crc.

For (3) I agree with your concern.

> There are a lot of supported ISA features, so there would be a lot of -m and 
> -mno- options to document. It would become harder to separate out -m options 
> related to architecture selection from -m options that do other things.

A rough count:

  $ clang --help | grep " \-m" | wc -l
  108

I also wouldn't want to get into a situation where we name an extension such 
that `-mextension` looks more like it's a general compiler option. E.g.

  -mnvj   Enable generation of new-value jumps

"nvj" isn't far off what our extensions end up being called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

[David Spickett via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe428baf0019e: [LLVM][ARM] Remove options for armv2, 2A, 3 
and 3M (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133109

Files:
  clang/docs/ReleaseNotes.rst
  llvm/docs/ReleaseNotes.rst
  llvm/include/llvm/Support/ARMTargetParser.def
  llvm/lib/Support/ARMTargetParser.cpp
  llvm/lib/Target/ARM/ARM.td
  llvm/lib/Target/ARM/ARMSubtarget.h
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/test/MC/ARM/directive-arch-armv2.s
  llvm/test/MC/ARM/directive-arch-armv2a.s
  llvm/test/MC/ARM/directive-arch-armv3.s
  llvm/test/MC/ARM/directive-arch-armv3m.s
  llvm/unittests/Support/TargetParserTest.cpp

Index: llvm/unittests/Support/TargetParserTest.cpp
===
--- llvm/unittests/Support/TargetParserTest.cpp
+++ llvm/unittests/Support/TargetParserTest.cpp
@@ -20,21 +20,20 @@
 
 namespace {
 const char *ARMArch[] = {
-"armv2",   "armv2a", "armv3",   "armv3m","armv4",
-"armv4t",  "armv5",  "armv5t",  "armv5e","armv5te",
-"armv5tej","armv6",  "armv6j",  "armv6k","armv6hl",
-"armv6t2", "armv6kz","armv6z",  "armv6zk",   "armv6-m",
-"armv6m",  "armv6sm","armv6s-m","armv7-a",   "armv7",
-"armv7a",  "armv7ve","armv7hl", "armv7l","armv7-r",
-"armv7r",  "armv7-m","armv7m",  "armv7k","armv7s",
-"armv7e-m","armv7em","armv8-a", "armv8", "armv8a",
-"armv8l",  "armv8.1-a",  "armv8.1a","armv8.2-a", "armv8.2a",
-"armv8.3-a",   "armv8.3a",   "armv8.4-a",   "armv8.4a",  "armv8.5-a",
-"armv8.5a","armv8.6-a",  "armv8.6a","armv8.7-a", "armv8.7a",
-"armv8.8-a",   "armv8.8a",   "armv8-r", "armv8r","armv8-m.base",
-"armv8m.base", "armv8-m.main",   "armv8m.main", "iwmmxt","iwmmxt2",
-"xscale",  "armv8.1-m.main", "armv9-a", "armv9", "armv9a",
-"armv9.1-a",   "armv9.1a",   "armv9.2-a",   "armv9.2a",
+"armv4","armv4t",  "armv5",  "armv5t",  "armv5e",
+"armv5te",  "armv5tej","armv6",  "armv6j",  "armv6k",
+"armv6hl",  "armv6t2", "armv6kz","armv6z",  "armv6zk",
+"armv6-m",  "armv6m",  "armv6sm","armv6s-m","armv7-a",
+"armv7","armv7a",  "armv7ve","armv7hl", "armv7l",
+"armv7-r",  "armv7r",  "armv7-m","armv7m",  "armv7k",
+"armv7s",   "armv7e-m","armv7em","armv8-a", "armv8",
+"armv8a",   "armv8l",  "armv8.1-a",  "armv8.1a","armv8.2-a",
+"armv8.2a", "armv8.3-a",   "armv8.3a",   "armv8.4-a",   "armv8.4a",
+"armv8.5-a","armv8.5a","armv8.6-a",  "armv8.6a","armv8.7-a",
+"armv8.7a", "armv8.8-a",   "armv8.8a",   "armv8-r", "armv8r",
+"armv8-m.base", "armv8m.base", "armv8-m.main",   "armv8m.main", "iwmmxt",
+"iwmmxt2",  "xscale",  "armv8.1-m.main", "armv9-a", "armv9",
+"armv9a",   "armv9.1-a",   "armv9.1a",   "armv9.2-a",   "armv9.2a",
 };
 
 template 
@@ -438,14 +437,6 @@
 }
 
 TEST(TargetParserTest, testARMArch) {
-  EXPECT_TRUE(
-  testARMArch("armv2", "generic", "v2", ARMBuildAttrs::CPUArch::Pre_v4));
-  EXPECT_TRUE(
-  testARMArch("armv2a", "generic", "v2a", ARMBuildAttrs::CPUArch::Pre_v4));
-  EXPECT_TRUE(
-  testARMArch("armv3", "generic", "v3", ARMBuildAttrs::CPUArch::Pre_v4));
-  EXPECT_TRUE(
-  testARMArch("armv3m", "generic", "v3m", ARMBuildAttrs::CPUArch::Pre_v4));
   EXPECT_TRUE(
   testARMArch("armv4", "strongarm", "v4",
   ARMBuildAttrs::CPUArch::v4));
@@ -603,10 +594,6 @@
   EXPECT_FALSE(testARMExtension("xscale", ARM::ArchKind::INVALID, "crc"));
   EXPECT_FALSE(testARMExtension("swift", ARM::ArchKind::INVALID, "crc"));
 
-  EXPECT_FALSE(testARMExtension("generic", ARM::ArchKind::ARMV2, "thumb"));
-  EXPECT_FALSE(testARMExtension("generic", ARM::ArchKind::ARMV2A, "thumb"));
-  EXPECT_FALSE(testARMExtension("generic", ARM::ArchKind::ARMV3, "thumb"));
-  EXPECT_FALSE(testARMExtension("generic", ARM::ArchKind::ARMV3M, "thumb"));
   EXPECT_FALSE(testARMExtension("generic", ARM::ArchKind::ARMV4, "dsp"));
   EXPECT_FALSE(testARMExtension("generic", ARM::ArchKind::ARMV4T, "dsp"));
   EXPECT_FALSE(testARMExtension("generic", ARM::ArchKind::ARMV5T, "simd"));
Index: llvm/test/MC/ARM/directive-arch-armv3m.s
===
--- llvm/test/MC/ARM/directive-arch-armv3m.s
+++ /dev/null
@@ -1,30 +0,0 @@
-@ Test the .arch directive for armv3m
-
-@ This test case will check the default .ARM.attributes value for the
-@ armv3m architecture.
-
-@ RUN: llvm-m