[Lldb-commits] [PATCH] D151567: [LLVM][Support] Report EISDIR when opening a directory on AIX

[Fangrui Song via Phabricator via lldb-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

Thanks for the patch, but there are two issues that should be fixed:

(1) stat => fstat
(2) change the subject to mean that this is not AIX specific (`[LLVM][Support] 
Report EISDIR when opening a directory on AIX`). Other OSes including Linux are 
changed as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151567

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

[Lldb-commits] [PATCH] D157497: feat: Migrate isArch16Bit

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D157497#4584621 , @Pivnoy wrote:

> This discussion was the main motivation for this change.
> https://discourse.llvm.org/t/rfc-refactor-triple-related-classes/70410/11
> As a result, Triple should become a data class, and its dependencies such as 
> Architecture, Operating System etc. represent in the form of interfaces that 
> can be implemented for the necessary instances.

FWIW I still don't see advantages by switching to the new 
`llvm::TripleUtils::isArch32Bit` style. How is it better than the current way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157497

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

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lld/ELF/Driver.h:28
   OPT_INVALID = 0,
-#define OPTION(_1, _2, ID, _4, _5, _6, _7, _8, _9, _10, _11, _12) OPT_##ID,
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
 #include "Options.inc"

lld/wasm lld/COFF lld/MachO are not updated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Fangrui Song via Phabricator via lldb-commits]

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

In D157028#4558724 , @jansvoboda11 
wrote:

> Here's an example of a patch that changes the `OPTION` macro: D157029 
> . I wonder if we could have counterparts to 
> `LLVM_MAKE_OPT_ID` and `LLVM_CONSTRUCT_OPT_INFO` that allow overriding the 
> default `OPT_` prefix. That would make D157029 
>  even smaller. WDYT @MaskRay?

The `#define OPTION(...) LLVM_MAKE_OPT_ID_WITH_ID_PREFIX(COFF_OPT_, 
__VA_ARGS__),` use case looks good:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I'll be away for a few days but I took a quick glance. This change looks 
reasonable. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157028

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

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp:23
+
+class TestMCDisasmInstanceRISCV : public testing::Test {
+public:

Place all classes and test methods in an anonymous namespace.



Comment at: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp:64
+  // and we should skip these tests without marking anything as failing.
+  if (disass_sp) {
+const InstructionList inst_list (disass_sp->GetInstructionList());

Early return



Comment at: llvm/include/llvm/MC/MCInstrAnalysis.h:76
+  return false;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;

`return Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156086

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

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

It seems that a lldb specific test is needed. Adding a new method to 
`llvm/include/llvm/MC/MCInstrAnalysis.h` is fine with me, though I haven't 
checked the semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156086

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

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Reverse ping @philnik :)

The `release/17.x` branch will be created soon 
(https://discourse.llvm.org/t/llvm-17-0-0-release-planning-and-update/71762).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151683

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

[Lldb-commits] [PATCH] D155178: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 6)

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D155178#4499105 , @MaskRay wrote:

> In D155178#4498911 , @thakis wrote:
>
>> http://45.33.8.238/linux/112305/step_4.txt looks like a missing include
>
> I've fixed it in 816141ce0eb899178dbcb6f0671875eb825b2f84 
>  but it 
> seems that the current build strategy of llvm-debuginfod makes it error-prone 
> for refactoring. @mysterymath  @phosek

Actually I am unable to figure out how to get brotli cmake files. I relied on 
gn to test my fix:)

  llvm/utils/gn/gn.py gen /tmp/out/gn
  ninja -C /tmp/out/gn llvm-debuginfod


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155178

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

[Lldb-commits] [PATCH] D155178: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 6)

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added subscribers: mysterymath, phosek.
MaskRay added a comment.

In D155178#4498911 , @thakis wrote:

> http://45.33.8.238/linux/112305/step_4.txt looks like a missing include

I've fixed it in 816141ce0eb899178dbcb6f0671875eb825b2f84 
 but it 
seems that the current build strategy of llvm-debuginfod made it error-prone 
for refactoring. @mysterymath  @phosek


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155178

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

[Lldb-commits] [PATCH] D155018: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 5)

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Still, would be good to land the lldb commit separately. And ensure that you 
have performed the last-minute testing...
Ensure that projects like `flang bolt` still build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155018

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

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D151683#4486321 , @aaron.ballman 
wrote:

> Aside from the failing precommit CI test case in C, I think this LGTM. I've 
> added @MaskRay as the code owner for the command line option changes in case 
> he's got concerns regarding the deprecation/removal plans.



  - ``-fdouble-square-bracket-attributes`` has been deprecated. It is ignored 
now
and will be removed in CLang 18.

sounds good. (Minor case typo in `CLang`). As you said in

https://discourse.llvm.org/t/rfc-enable-c-11-c2x-attributes-in-all-standard-modes-as-an-extension-and-remove-fdouble-square-bracket-attributes/71268/2

> I’m in support of this idea. I think we should enable the extension 
> unconditionally for Clang 17 with a release note mentioning that 
> -fdouble-square-bracket-attributes will be removed in Clang 18 just as a 
> kindness to users with proprietary code bases that might be using it.

I think a clear summary/commit message should mention that this patch makes 
`[[...]]`` available to C++03 and C17 with no warning by default.

It's also worth calling out that GCC since 10 supports `[[]]` in all C language 
modes (AFAICT there is no warning even with `gcc -std=c89 -c a.c`) (there is a 
warning `-pedantic`).

  % g++ -c a.cc -std=c++03
  a.cc:2:1: error: expected unqualified-id before ‘[’ token
  2 | [[nodiscard]] int without_underscores(void);
| ^
  % myclang++ -c a.cc -std=c++03  # no warning with this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151683

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

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Hi! The added lines have some spaces at line ends. I think using 
`clang-format-diff.py` would remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542

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

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D151683#4409633 , @aaron.ballman 
wrote:

> In D151683#4384017 , @erichkeane 
> wrote:
>
>> In D151683#4382408 , @philnik 
>> wrote:
>>
>>> No. I guess, since you are asking I should write one for this? Only for the 
>>> removal of `-fdouble-square-bracket-attributes`, or also for back porting 
>>> this?
>>
>> The RFC I would expect for "allow C/C++ style attributes as an extension in 
>> previous modes".  This is a pretty significant feature to allow as an 
>> extension, particularly since our two main alternative compilers (G++/MSVC) 
>> don't have this as an extension. I'd be curious how the other C compilers do 
>> this as well.
>
> I think this does warrant an RFC because it impacts both C++ and C, but I'm 
> hoping the RFC is uncontroversial. It's mostly to establish what the use 
> cases are for needing the extension. The feature is conforming as an 
> extension to both languages, and I don't know of any breakage that would come 
> from enabling it by default. I'm not certain whether we want to remove the 
> feature flag immediately or whether we'd like to give one release of warning 
> about it being removed (I'll defer to whatever @MaskRay thinks is reasonable) 
> -- that search is compelling for why it's safe to remove the option, but 
> there's plenty of proprietary code which we don't know about, so it's 
> possible the option is still used. I'd be especially curious to know if 
> anyone is using `-fno-double-square-bracket-attributes` to disable the 
> feature in a mode where it would otherwise be enabled. I'm adding the 
> `clang-vendors` group as a subscriber as an early warning that removing the 
> command line option could be a potentially breaking change.
>
> In terms of implementation work, there's still some minor stuff to address. 
> Also, please be sure to also add a release note about the changes, and a 
> potentially breaking entry for removing the command line option (assuming we 
> elect to go that route).

If allowing the extension in older language modes deems good, removing 
`-fno-double-square-bracket-attributes` seems fine if (and thanks for CCing the 
`clang-vendors` group).
If we want to play safely, we can default `-fdouble-square-bracket-attributes` 
to true in this patch and remove the option in a follow-up change possibly 
after some time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151683

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D141907#4094748 , @MaskRay wrote:

> [...]
> edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable 
> `CLANG_RESOURCE_DIR` but did not explain why. 
> In the long term, the CMake variable `CLANG_RESOURCE_DIR` probably should be 
> removed.

My feeling stays the same. In the long term, we should try removing the CMake 
variable `CLANG_RESOURCE_DIR`.
Customizing the variable in a special way will make some `clang/test/Driver` 
tests fail, I don't know it's healthy for contributors to be aware of this 
yet-another configure variable for `clang/test/Driver` tests. The 
`CLANG_RESOURCE_DIR` users should be served by specifying `-resource-dir=` in a 
configuration file 
(https://clang.llvm.org/docs/UsersManual.html#configuration-files)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

[Fangrui Song via Phabricator via lldb-commits]

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

If `nonMicrosoftDemangle` is going to be changed to take a `string_view` 
argument, adding `if (!MangledName) return false;` should be fine.
If possible, it's best for the change to be deferred to a later patch on the 
stack, but I won't push strongly on this. You did the work...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151003

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

[Lldb-commits] [PATCH] D149784: [Damangle] convert rustDemangle to use std::string_view

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/Demangle/RustDemangle.cpp:150
-char *llvm::rustDemangle(const char *MangledName) {
-  if (MangledName == nullptr)
-return nullptr;

I see that this patch drops `if (MangledName == nullptr)` (D101444). This is a 
right direction. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149784

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

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/Demangle/Demangle.cpp:49
 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string ) {
+  if (!MangledName)
+return false;

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > MaskRay wrote:
> > > Why is this change? The original contract is that `MangledName` must be 
> > > non-null.
> > https://fosstodon.org/@llvm/110397650834821908
> > 
> > It's insidious; implicitly constructing a `std::string_view` from a `char*` 
> > which is `nullptr` invokes a call to `strlen` upon construction, which 
> > segfaults on my system.  Therefor, all of the `nullptr` checks need to be 
> > hoisted into the callers or stated another way exist at the boundary of 
> > `char*` to `std::string_view` API boundaries (such as right here).
> This is also why the `nullptr` input test case must be removed from 
> llvm/unittests/Demangle/DLangDemangleTest.cpp below.
It's usually code smell if a function taking a C string argument additionally 
accepts nullptr. 
Previously, passing `nullptr` to `nonMicrosoftDemangle` will crash as 
`MangledName[0]` is accessed. We should retain this strictness.

`ninja check-llvm-demangle` passes if I remove the 2 lines.

If future refactoring will possibly pass `nullptr` to `nonMicrosoftDemangle`, I 
think we should fix those call sites not to do that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151003

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

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/Demangle/Demangle.cpp:49
 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string ) {
+  if (!MangledName)
+return false;

Why is this change? The original contract is that `MangledName` must be 
non-null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151003

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

[Lldb-commits] [PATCH] D150996: LLVM_FALLTHROUGH => [[fallthrough]]. NFC

[Fangrui Song via Phabricator via lldb-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150996

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

[Lldb-commits] [PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

[Fangrui Song via Phabricator via lldb-commits]

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

Thank you for making another try for the treewide change (which is admittedly 
very painful and not many people do such work).
Can you include the original patch description to the summary? That is the main 
part and the message "This reverts commit " is secondary. Readers should not 
need to trace through the revert history to obtain the original motivation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151344

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

[Lldb-commits] [PATCH] D149784: [Damangle] convert rustDemangle to use std::string_view

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a subscriber: labath.
MaskRay added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/include/lldb/Utility/ConstString.h:186
   operator llvm::StringRef() const { return GetStringRef(); }
+  // Implicitly convert \class ConstString instances to \calss 
std::string_view.
+  operator std::string_view() const { return std::string_view(m_string, 
GetLength()); }

Perhaps I find the previous comment not useful as well `Implicitly convert 
\class ConstString instances to \class StringRef.`

If we want to emphasize that they are implicit conversions instead of explicit 
conversions (https://en.cppreference.com/w/cpp/language/cast_operator), a 
single comment `// Implicit conversion functions.` should suffice, covering all 
conversion functions.

@JDevlieghere @labath


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149784

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

[Lldb-commits] [PATCH] D148384: [Demangle] replace use of llvm::StringView w/ std::string_view

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D148384#4270505 , @dhoekwater 
wrote:

> It looks like this breaks the build: 
> https://lab.llvm.org/buildbot#builders/119/builds/12865 
> https://lab.llvm.org/buildbot#builders/123/builds/18388 
> https://lab.llvm.org/buildbot#builders/60/builds/11628
>
> I'm pretty new with LLVM so I'll leave reverting the change to someone else, 
> but I figured I should give you a heads up.

I am unsure about the Windows issue and find it difficult to repair...   I'm 
testing a revert commit to not leave issues to the weekend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148384

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

[Lldb-commits] [PATCH] D148384: [Demangle] replace use of llvm::StringView w/ std::string_view

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:19
 
+#include 
+

chapuni wrote:
> Odd layering...
Good catch. LLVMDemangle cannot depend on other LLVM libraries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148384

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

[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/include/lldb/lldb-private-types.h:14
 
+#include "lldb/Target/RegisterFlags.h"
 #include "lldb/lldb-private.h"

Is there a library layering violation?

I assume that `lldb/Target/*.h` files depend on 
`lldb/include/lldb/lldb-private-types.h`, so 
`lldb/include/lldb/lldb-private-types.h` cannot include `lldb/Target/*.h` 
files...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145574

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

[Lldb-commits] [PATCH] D132608: [CMake] Clean up CMake binary dir handling

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.
Herald added subscribers: bviyer, ekilmer, jplehr, thopre.

@Ericson2314  @phosek What's the state of this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132608

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

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

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D138539#4164313 , @steakhal wrote:

> Oh, I forgot to mention why this change breaks the equality operator of 
> `llvm::StringSet`. It's because the `StringMap::operator==` will try to 
> compare the `value` as well, which is of type `std::nullopt_t` and that has 
> no equality comparison operator.
> Previously, the `enum class NoneType` has a default one.

Since `std::unordered_set` and counterparts like `absl::flat_hash_set` provide 
`operator==`, I think providing `StringMap::operator==` is fine. I don't think 
whether the operation is commonly used, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

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

[Lldb-commits] [PATCH] D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

This seems to cause many lldb failures 
https://lab.llvm.org/buildbot/#/builders/68/builds/47790


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143652

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

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added inline comments.



Comment at: llvm/test/ObjectYAML/Offload/multiple_members.yaml:21
   Value:"gfx908"
 Content:  "cafefeed"  
 

Rebase:) I fixed obj2yaml



Comment at: llvm/test/tools/llvm-profdata/suppl-instr-with-sample.test:1
 Some basic tests for supplementing instrumentation profile with sample profile.
 

llvm-profdata uses OnDiskHashTable.h which I think is infeasible to stabilize 
without drastic change to the format. So keep the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142862

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

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

> `For posterity, the tests that fail on main are:`
>
> `Skipped  : 43`
> [...]

You can remove these from the description. They are flaky tests unrelated to 
this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142862

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

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.



Comment at: mlir/test/Transforms/print-op-graph.mlir:1
 // RUN: mlir-opt -allow-unregistered-dialect 
-mlir-elide-elementsattrs-if-larger=2 -view-op-graph %s -o %t 2>&1 | FileCheck 
-check-prefix=DFG %s
 // RUN: mlir-opt -allow-unregistered-dialect 
-mlir-elide-elementsattrs-if-larger=2 
-view-op-graph='print-data-flow-edges=false print-control-flow-edges=true' %s 
-o %t 2>&1 | FileCheck -check-prefix=CFG %s

I fixed `mlir/lib/Transforms/ViewOpGraph.cpp`. Please rebase and drop changes 
to this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142862

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

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:638
 
-  EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc"), testPath("A.h")));
+  EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.h"), testPath("A.cc")));
   // Make sure we only store the Cmd for main file.

Rebase. I've changed this to `UnorderedElementsAre`



Comment at: clang-tools-extra/test/modularize/ProblemsInconsistent.modularize:6
 
-# CHECK: error: macro 'SYMBOL' defined at multiple locations:
-# CHECK-NEXT: {{.*}}{{[/\\]}}Inputs{{[/\\]}}InconsistentSubHeader.h:3:9
-# CHECK-NEXT: {{.*}}{{[/\\]}}Inputs{{[/\\]}}InconsistentSubHeader.h:7:9
-# CHECK-NEXT: error: macro 'FUNC_STYLE' defined at multiple locations:
+# CHECK: error: macro 'FUNC_STYLE' defined at multiple locations:
 # CHECK-NEXT: {{.*}}{{[/\\]}}Inputs{{[/\\]}}InconsistentSubHeader.h:4:9

Rebase. I've fixed modularize's misuse of StringMap 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h

> StringMap iteration order, however, is not guaranteed to be deterministic, so 
> any uses which require that should instead use a std::map.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142862

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable 
`CLANG_RESOURCE_DIR` but did not explain why. 
The patch introduced conditions in C++ code like

  std::string path_to_clang_dir = std::string(CLANG_RESOURCE_DIR).empty()
  ? "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME
"/clang/" CLANG_VERSION_MAJOR_STRING
  : "/foo/bar/bin/" CLANG_RESOURCE_DIR;

which makes me uncomfortable.

I wish that we can just replace all places that construct 
`CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING` manually 
with a simple `CLANG_RESOURCE_DIR` which is guaranteed to be non-empty.

In the long term, the CMake variable `CLANG_RESOURCE_DIR` probably should be 
removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

To enable smooth hash function migration in the future, we can explore the idea 
of adding `#ifdef EXPENSIVE_CHECKS\nshuffle` (see 
https://github.com/llvm/llvm-project/issues/34483).
That means we should fix these tests properly to be non-dependent on the 
iteration order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142862

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

[Lldb-commits] [PATCH] D140999: [NFC][TargetParser] Deprecate llvm/Support/AArch64TargetParser.h

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.

I don't think it is necessary to deprecate the old header then delete it after 
16.0.0 is branched.
llvm/Support/AArch64TargetParser.h has very few open-source out-of-tree uses. 
Perhaps only ldc `driver/targetmachine.cpp` uses the header. So it is not worth 
extra expedience.

Changing the include to `#include "llvm/TargetParser/AArch64TargetParser.h"` is 
totally fine, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140999

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

[Lldb-commits] [PATCH] D140896: [WIP] Move from llvm::makeArrayRef to ArrayRef deduction guides

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.

LGTM if `makeArrayRef` is kept in this patch. Do we foresee compiler bugs (for 
supported compilers) in this area? If there may be a risk, consider migrate a 
part of the code base first.


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

https://reviews.llvm.org/D140896

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

[Lldb-commits] [PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Can you push `using OptionalFileEntryRef = CustomizableOptional;` 
and the renaming as a separate commit to make this patch smaller?
There is a smaller that this rename may be reverted.
205c0589f918f95d2f2c586a01bea2716d73d603 
 reverted 
a change related to `OptionalFileEntryRef`. I assume that your change is fine.




Comment at: mlir/include/mlir/IR/Visitors.h:49
   bool operator==(const WalkResult ) const { return result == rhs.result; }
+  bool operator!=(const WalkResult ) const { return result != rhs.result; }
 

Can be pushed separately


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.

Please check these builds all work:

- default
- `-DLLVM_LINK_LLVM_DYLIB=on`
- `-DBUILD_SHARED_LIBS=on`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The name `llvm/lib/TargetParser` LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Moving target-specific parsers outside LLVMSupport LGTM. I even objected a bit 
when more stuff of this kind was introduced into LLVMSupport.

+1 for adding temporary forwarding headers for now to avoid update all 
`#include` users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[Lldb-commits] [PATCH] D137724: [CMake] Warn when the version is older than 3.20.0.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision as: MaskRay.
MaskRay added a comment.

I think `if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)` checks for 
standalone builds is not necessary. The check in `llvm/CMakeLists.txt` suffices.
It's unlikely the users will use different cmake versions to configure llvm and 
a subproject like clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137724

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

[Lldb-commits] [PATCH] D137503: [CMake] Fix -Wstrict-prototypes

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D137503#3910651 , @aaron.ballman 
wrote:

> LGTM, thank you for this! If we do a 15.0.5, I think these changes should 
> probably go into there as well (WDYT?)

I support this. This will make llvm-project 15.0.5 buildable by more future 
compilers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137503

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

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.
Herald added a subscriber: Moerafaat.

> We held off on this before as `LLVM_LIBDIR_SUFFIX` conflicted with it. Now we 
> return this.
>
> I imagine this is too potentially-breaking to make LLVM 15. That's fine. ...

These sentences are no longer relevant and should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

zstd provides GNU Makefile, CMake, and Meson. The CMake files are only 
installed when configured with CMake. Debian and Ubuntu lack such files.
The pkg-config file libzstd.pc is probably the most portable file. (I have also 
used it for a binutils-gdb patch.)

I think we can then avoid the

  zstdConfig.cmake
  zstd-config.cmake

issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/include/llvm/Support/Compression.h:48
+
+constexpr int NoCompression = -5;
+constexpr int BestSpeedCompression = 1;

MaskRay wrote:
> I missed the values here. Why is -5 picked for NoCompression? What does it 
> mean?
> 
> zstd.h says ZSTD_maxCLevel() is currently 22. The CLI program mentions 19. 
> Why is BestSizeCompression 12?
> 
> ZSTD_CLEVEL_DEFAULT/the CLI CLI uses 3 for the default level. Why is 
> DefaultCompression 5?
Ping @ckissane about the choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/include/llvm/Support/Compression.h:48
+
+constexpr int NoCompression = -5;
+constexpr int BestSpeedCompression = 1;

I missed the values here. Why is -5 picked for NoCompression? What does it mean?

zstd.h says ZSTD_maxCLevel() is currently 22. The CLI program mentions 19. Why 
is BestSizeCompression 12?

ZSTD_CLEVEL_DEFAULT/the CLI CLI uses 3 for the default level. Why is 
DefaultCompression 5?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I am still seeing the

  zstdConfig.cmake
  zstd-config.cmake

warning. @ckissane @phosek will you fix it? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

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

Does this address the macOS build failure?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D138310: [NFC] Make headers self-contained.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.
Herald added a subscriber: StephenFan.



Comment at: lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h:15
 
-#include "EmulateInstructionRISCV.h"
 #include "llvm/ADT/Optional.h"

I assume that this is correct, even after considering IWYU. Perhaps worth a 
double check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138310

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

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

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:218
+  explicit NameLookup(std::nullopt_t) : Data(nullptr, true) {}
   explicit NameLookup(std::nullptr_t) : Data(nullptr, false) {}
   NameLookup() : NameLookup(nullptr) {}

A maintainer might want to check the uses. Having 3 constructors looks 
confusing to readers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138539

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

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D137217#3945366 , @glandium wrote:

> Almost there, but not quite:
>
>   [task 2022-11-22T23:55:36.341Z] 
> /builds/worker/fetches/llvm-project/llvm/tools/gold/gold-plugin.cpp:1106:6: 
> error: no matching function for call to object of type '(lambda at 
> /builds/worker/fetches/llvm-project/llvm/tools/gold/gold-plugin.cpp:1095:7)'
>   [task 2022-11-22T23:55:36.341Z] *AddStream(Task)->OS << MB->getBuffer();
>   [task 2022-11-22T23:55:36.341Z]  ^
>   [task 2022-11-22T23:55:36.341Z] 
> /builds/worker/fetches/llvm-project/llvm/tools/gold/gold-plugin.cpp:1095:7: 
> note: candidate function not viable: requires 2 arguments, but 1 was provided
>   [task 2022-11-22T23:55:36.341Z]   [&](size_t Task,
>   [task 2022-11-22T23:55:36.341Z]   ^
>   [task 2022-11-22T23:55:36.341Z] 1 error generated.

`ninja LLVMgold.so` works for me now. I think @zequanwu fixed this in 
10a43c4641c20e0a50edc0ff99915c837a507cc1 
/5d140dc2c0f3068fae7127b85df861c420848078
 
/d23b63ceccfe526c3a9b17d35032b3666e6816a1
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

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

[Lldb-commits] [PATCH] D138376: Use None consistently (NFC)

[Fangrui Song via Phabricator via lldb-commits]

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

LGTM. `enum class NoneType { None = 1 };` is from 
0cd22f9540c0591132ec991c51103cf800cf4e24 (2017) for MSVC workaround. I assume 
it is no longer needed..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138376

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

[Lldb-commits] [PATCH] D138276: TableGen: require tablegen cl::opts to be registered explicitly

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: StephenFan.

This is similar to `mlir::register*Options` and looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138276

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

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:846
+  auto AddStream = [&](size_t Task,
+   Twine ModuleName) -> std::unique_ptr {
 int FD = -1;

`Twine` should only be used as const references as arguments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

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

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lld/COFF/LTO.cpp:229
+StringRef ltoObjName;
+if (bitcodeFilePath == "ld-temp.o") {
+  ltoObjName =

MaskRay wrote:
> tejohnson wrote:
> > zequanwu wrote:
> > > tejohnson wrote:
> > > > This case should always be i==0 I think?
> > > IIUC, "ld-temp.o" is the name of combined module. Do you mean there will 
> > > be only 1 combined module and it will always be the first task?
> > Yes. So you don't need the handling for i==0 in the name in this case (you 
> > could probably assert that i==0).
> This looks like a hack. `assert(i == 0)` will fail with 
> `-opt:lldltopartitions=2`: `buf[1]` will be called `ld-temp.o` as well.
> 
> In addition, if an input bitcode file is called `ld-temp.o` (for some reason 
> they don't use `.obj`, just `ld-temp.o`), `assert(i == 0)` will fail as well.
I guess `if (i < config->ltoPartitions)` may fix the issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

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

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D137217#3926601 , @tejohnson wrote:

> @MaskRay wondering if this is a good change to make for ELF as well, wdyt?

Yes, I think this is a good idea and improves debuggability. The change is 
non-trivial so so this patch focusing on the COFF part is good.




Comment at: lld/COFF/LTO.cpp:238
+  sys::path::append(path, directory,
+outputFileBaseName + ".lto." + baseName);
+  sys::path::remove_dots(path, true);

What if two input bitcode files have the same basename, e.g. `dir1/a.obj` and 
`dir2/a.obj`?



Comment at: lld/COFF/LTO.cpp:229
+StringRef ltoObjName;
+if (bitcodeFilePath == "ld-temp.o") {
+  ltoObjName =

tejohnson wrote:
> zequanwu wrote:
> > tejohnson wrote:
> > > This case should always be i==0 I think?
> > IIUC, "ld-temp.o" is the name of combined module. Do you mean there will be 
> > only 1 combined module and it will always be the first task?
> Yes. So you don't need the handling for i==0 in the name in this case (you 
> could probably assert that i==0).
This looks like a hack. `assert(i == 0)` will fail with 
`-opt:lldltopartitions=2`: `buf[1]` will be called `ld-temp.o` as well.

In addition, if an input bitcode file is called `ld-temp.o` (for some reason 
they don't use `.obj`, just `ld-temp.o`), `assert(i == 0)` will fail as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

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

[Lldb-commits] [PATCH] D125860: [clang] Only use major version in resource dir

[Fangrui Song via Phabricator via lldb-commits]

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

LGTM. Considering 
https://discourse.llvm.org/t/should-we-continue-embed-the-full-llvm-version-in-lib-clang/62094
  and this thread, I think overall people favor this patch.

If a distribution wants to provide 16.0 and 16.1 simultaneously, the two 
installations can be in different install prefixes :)


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

https://reviews.llvm.org/D125860

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

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

It'll be good if we have a wiki describing how downstream users invoke cmake, 
so that large cmake refactoring can be verified beforehand.
(like usage verification 
https://github.com/opencollab/llvm-toolchain-integration-test-suite, but for 
cmake invocations)


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

https://reviews.llvm.org/D136572

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

[Lldb-commits] [PATCH] D135400: [clang][LTO] Remove the use of `--` for arange option

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/Driver/debug-options-aranges.c:6
 // RUN: %clang -### -g --target=x86_64-linux -flto=thin -gdwarf-aranges %s 
2>&1 | FileCheck %s
-// CHECK: --plugin-opt=-generate-arange-section
+// CHECK: -plugin-opt=-generate-arange-section




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

https://reviews.llvm.org/D135400

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

[Lldb-commits] [PATCH] D135400: [clang][LTO] Remove the use of `--` for arange option

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I missed https://reviews.llvm.org/D134668 . See my comment there I don't think 
the change is necessary.


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

https://reviews.llvm.org/D135400

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

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

D134385  should fix the problem:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133525

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

[Lldb-commits] [PATCH] D132300: [clang][lldb][cmake] Use new `*_INSTALL_LIBDIR_BASENAME` CPP macro

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In the long term we should just remove the `CLANG_INSTALL_LIBDIR_BASENAME` 
customization. This is supposed for GCC multilib lib32 lib64 names but we don't 
necessarily use it for Clang + compiler-rt files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132300

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

[Lldb-commits] [PATCH] D133530: [lldb] Add zstd support

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Ping for unresolved issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133530

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

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:509
+  const ElfType OutputElfType =
+  getOutputElfType(Config.OutputArch.value_or(MachineInfo()));
+  const bool Is64Bit =

This is incorrect. Config.OutputArch is usually unset (`-O`).

We need a field in `Object` and initialize it in 
`llvm/lib/ObjCopy/ELF/ELFObject.cpp:1883`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133525

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

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

See `test/tools/llvm-objcopy/ELF/compress-debug-sections-zstd.test`. Use a 
similar file for ELFCLASS32 which runs `llvm-objcopy 
--compress-debug-sections=zstd` then  `llvm-objcopy 
--decompress-debug-sections`. Then compare the output with the original with 
just one `llvm-objcopy` run.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133525

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

[Lldb-commits] [PATCH] D133530: [lldb] Add zstd support

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/test/Shell/ObjectFile/ELF/compressed-sections-zstd.yaml:19
+Content: deadbeefbaadf00d
+## The legacy .zdebug format is not supported.
+  - Name:.zdebug_info

Delete `.zdebug`. It is unrelated to zstd



Comment at: lldb/test/Shell/ObjectFile/ELF/compressed-sections-zstd.yaml:26
+# CHECK-NEXT: Type: regular
+# CHECK: VM address: 0
+# CHECK-NEXT: VM size: 0

Align



Comment at: llvm/lib/Object/Decompressor.cpp:48
+  if (ELFCompressionSchemeId == ELFCOMPRESS_ZSTD)
+CompressionType = llvm::DebugCompressionType::Zstd;
+

Use a switch so that in the case of new compression format, the code get 
notified  due to -Wswitch



Comment at: llvm/lib/Object/Decompressor.cpp:52
 return createError("unsupported compression type");
+  if (llvm::compression::getReasonIfUnsupported(
+  compression::formatFor(CompressionType)) != nullptr) {

Just use the return type of `getReasonIfUnsupported` as the error message



Comment at: llvm/lib/Object/Decompressor.cpp:74
   size_t Size = Buffer.size();
-  return compression::zlib::uncompress(arrayRefFromStringRef(SectionData),
-   Buffer.data(), Size);
+  return compression::decompress(compression::formatFor(CompressionType),
+ arrayRefFromStringRef(SectionData),




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133530

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

[Lldb-commits] [PATCH] D111509: [clang] use getCommonSugar in an assortment of places

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D111509#3778882 , @mizvekov wrote:

> In D111509#3778879 , @MaskRay wrote:
>
>> Since the fix was not merged, I will revert this soon.
>>
>> I saw a clang segfault with `clang++ libstdc++-v3/src/c++98/complex_io.cc`
>
> Would you mind if I just merge it? I was hoping there would be someone around 
> to have a quick look, but apparently not.

Sorry, I just reverted it:) You may check whether the fix fixes GCC 
libstdc++-v3/src/c++98/complex_io.cc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111509

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

[Lldb-commits] [PATCH] D111509: [clang] use getCommonSugar in an assortment of places

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Since the fix was not merged, I will revert this soon.

I saw a clang segfault with `clang++ libstdc++-v3/src/c++98/complex_io.cc`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111509

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D130689#3710291 , @aaron.ballman 
wrote:

> In D130689#3710281 , @royjacobson 
> wrote:
>
>> In D130689#3709834 , @thieta wrote:
>>
>>> In D130689#3709742 , 
>>> @aaron.ballman wrote:
>>>
 One thing I think would be a definite improvement is to have done an RFC 
 on Discourse for these changes so that downstreams have a chance to weigh 
 in on the impact. The patch was put up on Jul 28 and landed about a week 
 later without any notification to the rest of the community who might not 
 be watching cfe-commits -- that's a very fast turnaround and very little 
 notification for such a significant change.
>>>
>>> Yeah this is on me. Honestly I didn't expect it to be that much of a 
>>> problem but rather the toolchain requirement we posted as part of it would 
>>> be the big hurdle where bot owners would have to upgrade to get the right 
>>> versions. But lesson learned  and we should add some more delays in the 
>>> policy here: https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the 
>>> C++ standards upgrade.
>>
>> Two points I want to add that I think would've been useful as well -
>>
>> 1. In addition to the toolchain soft errors, add a version check + #warning 
>> to some llvm header. This would be useful as it is more visible than the 
>> CMake warning and it could show up for cases where LLVM is used as a 
>> library+headers and not built from sources.
>> 2. Delay actual usage of new language features until after the next release. 
>> Currently I see people pushing lots of cleanup commits that could hurt bug 
>> backports. It also has the benefit of making the transition more gradual.
>
> Strong +1 to point #2 especially. This is something we could have plausibly 
> reverted to work through the kinks rather than doing that work live and while 
> under duress, but it became implausible pretty quickly because everyone 
> started landing their C++17 NFC changes. Those kinds of changes almost always 
> can wait until after we've validated that the switch has gone smoothly.

Point #2 can be advised but may not have too much a difference. I work on a 
large monolithic code base and have good experience that people quickly use new 
features (sometimes inadvertently) with a new release of clang/mlir/etc or 
use/stick with an unsupported use for an extended period of time. It's very 
difficult to prevent either activity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

[Fangrui Song via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcdeb50c32155: [lldb] Remove include/lldb/lldb-private.h 
(authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131422

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/include/lldb/lldb-private-defines.h
  lldb/include/lldb/lldb-private.h
  lldb/include/lldb/module.modulemap


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" 
export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * 
}
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h --*- C++ 
-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef LLDB_LLDB_PRIVATE_DEFINES_H
-#define LLDB_LLDB_PRIVATE_DEFINES_H
-
-#if defined(__cplusplus)
-
-// Include Compiler.h here so we don't define LLVM_FALLTHROUGH and then
-// Compiler.h later tries to redefine it.
-#include "llvm/Support/Compiler.h"
-
-#ifndef LLVM_FALLTHROUGH
-
-#ifndef __has_cpp_attribute
-#define __has_cpp_attribute(x) 0
-#endif
-
-/// \macro LLVM_FALLTHROUGH
-/// Marks an empty statement preceding a deliberate switch fallthrough.
-#if __has_cpp_attribute(clang::fallthrough)
-#define LLVM_FALLTHROUGH [[clang::fallthrough]]
-#else
-#define LLVM_FALLTHROUGH
-#endif
-
-#endif // ifndef LLVM_FALLTHROUGH
-
-#endif // #if defined(__cplusplus)
-
-#endif // LLDB_LLDB_PRIVATE_DEFINES_H
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -3591,7 +3591,6 @@
 lldb/examples/synthetic/bitfield/program.cpp
 lldb/include/lldb/lldb-defines.h
 lldb/include/lldb/lldb-forward.h
-lldb/include/lldb/lldb-private-defines.h
 lldb/include/lldb/lldb-private.h
 lldb/include/lldb/lldb-public.h
 lldb/include/lldb/lldb-versioning.h


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * }
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h --*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// 

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

[Fangrui Song via Phabricator via lldb-commits]

MaskRay created this revision.
MaskRay added reviewers: JDevlieghere, kastiglione.
Herald added a subscriber: StephenFan.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

The header from 62e0681afb478a4005efb6ba3598c24dc24866ee does something with
LLVM_FALLTHROUGH. Now that llvm-project has switched to C++17 and
LLVM_FALLTHROUGH uses have been migrated to [[fallthrough]], the header is
unneeded.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131422

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/include/lldb/lldb-private-defines.h
  lldb/include/lldb/lldb-private.h
  lldb/include/lldb/module.modulemap


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" 
export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * 
}
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h --*- C++ 
-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef LLDB_LLDB_PRIVATE_DEFINES_H
-#define LLDB_LLDB_PRIVATE_DEFINES_H
-
-#if defined(__cplusplus)
-
-// Include Compiler.h here so we don't define LLVM_FALLTHROUGH and then
-// Compiler.h later tries to redefine it.
-#include "llvm/Support/Compiler.h"
-
-#ifndef LLVM_FALLTHROUGH
-
-#ifndef __has_cpp_attribute
-#define __has_cpp_attribute(x) 0
-#endif
-
-/// \macro LLVM_FALLTHROUGH
-/// Marks an empty statement preceding a deliberate switch fallthrough.
-#if __has_cpp_attribute(clang::fallthrough)
-#define LLVM_FALLTHROUGH [[clang::fallthrough]]
-#else
-#define LLVM_FALLTHROUGH
-#endif
-
-#endif // ifndef LLVM_FALLTHROUGH
-
-#endif // #if defined(__cplusplus)
-
-#endif // LLDB_LLDB_PRIVATE_DEFINES_H
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -3591,7 +3591,6 @@
 lldb/examples/synthetic/bitfield/program.cpp
 lldb/include/lldb/lldb-defines.h
 lldb/include/lldb/lldb-forward.h
-lldb/include/lldb/lldb-private-defines.h
 lldb/include/lldb/lldb-private.h
 lldb/include/lldb/lldb-public.h
 lldb/include/lldb/lldb-versioning.h


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * }
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h 

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D130689#3704581 , @dyung wrote:

> Your change is causing a build failure on the PS4 linux build bot using GCC 
> 9.3. Can you take a look?
> https://lab.llvm.org/buildbot/#/builders/139/builds/26186
>
>   FAILED: 
> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o
>  
>   CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/g++ 
> -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS 
> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
> -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/clang-tidy/bugprone
>  
> -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone
>  
> -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/clang-tidy
>  
> -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang/include
>  
> -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/include
>  
> -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/include
>  
> -I/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/llvm/include
>  -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden 
> -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings 
> -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long 
> -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess 
> -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type 
> -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment 
> -Wmisleading-indentation -fdiagnostics-color -ffunction-sections 
> -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 
> -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT 
> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o
>  -MF 
> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o.d
>  -o 
> tools/clang/tools/extra/clang-tidy/bugprone/CMakeFiles/obj.clangTidyBugproneModule.dir/SignalHandlerCheck.cpp.o
>  -c 
> /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
>   
> /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:17:45:
>  error: modification of ‘’ is not a constant expression
>  17 | "signal", "abort", "_Exit", "quick_exit"};
> | ^
>   
> /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:217:12:
>  error: modification of ‘’ is not a constant expression
> 217 | "write"};
> |^

Fixed by c7ec86b13c461f6a8ce11f8443c1b6242013d26f 
 .
May be related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102921


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D128465: [OLD] [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128465#3660733 , @ckissane wrote:

> In D128465#3660714 , @MaskRay wrote:
>
>> Instead of marking a differential `[OLD] ` (which may confuse readers 
>> whether this is to be abandoned), you can mark a differential `Request 
>> Reviews`/`Request Changes`. Then it will not be in the green state.
>>
>> I clicked "Reopen" to make it clear the patch hasn't relanded.
>
> Oh, sorry about that!

That's fine :) We need to learn the web UI.

> I had actually created https://reviews.llvm.org/D129786 instead of reopening 
> this one because at the time I tried, reopening didn't seem to work.

A reverted patch by default is still in the "Closed" state. You can "reopen" it 
so that people know it hasn't relanded.
When repairing a differential, just reuse the existing patch. It keeps all 
discussions (including past suggestions and issues a previous commit did not 
address) in one thread.
If the new patch is very similar to this one, you probably need to abandon it 
in favor of this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [OLD] [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Instead of marking a differential `[OLD] ` (which may confuse readers whether 
this is to be abandoned), you can mark a differential `Request 
Reviews`/`Request Changes`. Then it will not be in the green state.

I clicked "Reopen" to make it clear the patch hasn't relanded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D129724: [lldb] Remove ELF .zdebug support

[Fangrui Song via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.
Closed by commit rGecfaf4801cd0: [lldb] Remove ELF .zdebug support (authored by 
MaskRay).

Changed prior to commit:
  https://reviews.llvm.org/D129724?vs=444504=444707#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129724

Files:
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
  lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml

Index: lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml
+++ /dev/null
@@ -1,56 +0,0 @@
-## NB: This is a yaml file because llvm gnu-style writing support in 14.0
-## (2022, D117744). In due time we may want to remove it from lldb as well.
-
-## Debug info generated from the following sources using clang-13
-## struct A {
-##  long a = 42;
-## };
-## extern constexpr short s = 47;
-## extern constexpr A a{};
-
-# REQUIRES: zlib
-
-# RUN: yaml2obj %s > %t
-# RUN: %lldb %t -o "target var s a" -b | FileCheck %s
-
-# CHECK: (const short) s = 47
-# CHECK: (const A) a = (a = 42)
-
 !ELF
-FileHeader:
-  Class:   ELFCLASS64
-  Data:ELFDATA2LSB
-  Type:ET_EXEC
-  Machine: EM_X86_64
-  Entry:   0x401000
-Sections:
-  - Name:.rodata
-Type:SHT_PROGBITS
-Flags:   [ SHF_ALLOC ]
-Address: 0x401000
-AddressAlign:0x8
-Offset:  0x1000
-Content: 2F002A00
-  - Name:.zdebug_info
-Type:SHT_PROGBITS
-AddressAlign:0x1
-Content: 5A4C49420077789C2B666060606100010E4610A9C820CA00012640CCE407248C81989197939941C0012CC16C01D2130024589998C280540848011F2733074C4124488E35116C28171B48493E480937505B04488A830100368605A7
-  - Name:.debug_abbrev
-Type:SHT_PROGBITS
-AddressAlign:0x1
-Content: 011101250E1305030E10171B0E023400030E49133F193A0B3B0B02180326004913042400030E3E0B0B0B051301360B030E0B0B3A0B3B0B060D00030E49133A0B3B0B380B00
-  - Name:.debug_line
-Type:SHT_PROGBITS
-AddressAlign:0x1
-Content: 3C0004003600010101FB0E0D00010101010001012F746D70676E752D7374796C652D636F6D7072657373696F6E2E63707100
-DWARF:
-  debug_str:
-- clang version 13.0.0
-- '/tmp/gnu-style-compression.cpp'
-- '/tmp/my_working_directory'
-- s
-- short
-- a
-- long int
-- A
-...
Index: lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
===
--- lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
+++ lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
@@ -16,6 +16,7 @@
 Type:SHT_PROGBITS
 Flags:   [ SHF_COMPRESSED ]
 Content: deadbeefbaadf00d
+## The legacy .zdebug format is not supported.
   - Name:.zdebug_info
 Type:SHT_PROGBITS
 Content: 5A4C49420008789c533070084828689809c802c1
@@ -37,8 +38,8 @@
 # CHECK-NEXT: Data: ()
 
 # CHECK: Name: .zdebug_info
-# CHECK: dwarf-info
+# CHECK: regular
 # CHECK: File size: 28
 # CHECK: Data: (
-# CHECK-NEXT: 20304050 60708090
+# CHECK-NEXT: 5A4C4942  0008 789C5330 70084828 6898 09C802C1
 # CHECK-NEXT: )
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1607,7 +1607,7 @@
 }
 
 static SectionType GetSectionTypeFromName(llvm::StringRef Name) {
-  if (Name.consume_front(".debug_") || Name.consume_front(".zdebug_")) {
+  if (Name.consume_front(".debug_")) {
 return llvm::StringSwitch(Name)
 .Case("abbrev", eSectionTypeDWARFDebugAbbrev)
 .Case("abbrev.dwo", eSectionTypeDWARFDebugAbbrevDwo)
@@ -3365,8 +3365,7 @@
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
   size_t result = ObjectFile::ReadSectionData(section, section_data);
-  if (result == 0 || !llvm::object::Decompressor::isCompressedELFSection(
- section->Get(), section->GetName().GetStringRef()))
+  if (result == 0 || !(section->Get() & llvm::ELF::SHF_COMPRESSED))
 return result;
 
   auto Decompressor = llvm::object::Decompressor::create(
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129724: [lldb] Remove ELF .zdebug support

[Fangrui Song via Phabricator via lldb-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1610
 static SectionType GetSectionTypeFromName(llvm::StringRef Name) {
   if (Name.consume_front(".debug_") || Name.consume_front(".zdebug_")) {
 return llvm::StringSwitch(Name)

labath wrote:
> I guess you can remove this as well now.
Thanks. Will delete when committing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129724

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

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Why was this reverted? Please make extensive tests especially for something 
dealing with the build system. When reverting a commit, briefly describe what 
happened.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D129724: [lldb] Remove ELF .zdebug support

[Fangrui Song via Phabricator via lldb-commits]

MaskRay created this revision.
MaskRay added reviewers: ckissane, labath.
Herald added subscribers: StephenFan, emaste.
Herald added a reviewer: alexander-shaposhnikov.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

clang 14 removed -gz=zlib-gnu support and ld.lld/llvm-objcopy removed zlib-gnu
support recently. Remove lldb support by migrating away from
llvm::object::Decompressor::isCompressedELFSection.
The API has another user llvm-dwp, so it is not removed in this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129724

Files:
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
  lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml


Index: lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml
+++ /dev/null
@@ -1,56 +0,0 @@
-## NB: This is a yaml file because llvm gnu-style writing support in 14.0
-## (2022, D117744). In due time we may want to remove it from lldb as well.
-
-## Debug info generated from the following sources using clang-13
-## struct A {
-##  long a = 42;
-## };
-## extern constexpr short s = 47;
-## extern constexpr A a{};
-
-# REQUIRES: zlib
-
-# RUN: yaml2obj %s > %t
-# RUN: %lldb %t -o "target var s a" -b | FileCheck %s
-
-# CHECK: (const short) s = 47
-# CHECK: (const A) a = (a = 42)
-
 !ELF
-FileHeader:
-  Class:   ELFCLASS64
-  Data:ELFDATA2LSB
-  Type:ET_EXEC
-  Machine: EM_X86_64
-  Entry:   0x401000
-Sections:
-  - Name:.rodata
-Type:SHT_PROGBITS
-Flags:   [ SHF_ALLOC ]
-Address: 0x401000
-AddressAlign:0x8
-Offset:  0x1000
-Content: 2F002A00
-  - Name:.zdebug_info
-Type:SHT_PROGBITS
-AddressAlign:0x1
-Content: 
5A4C49420077789C2B666060606100010E4610A9C820CA00012640CCE407248C81989197939941C0012CC16C01D2130024589998C280540848011F2733074C4124488E35116C28171B48493E480937505B04488A830100368605A7
-  - Name:.debug_abbrev
-Type:SHT_PROGBITS
-AddressAlign:0x1
-Content: 
011101250E1305030E10171B0E023400030E49133F193A0B3B0B02180326004913042400030E3E0B0B0B051301360B030E0B0B3A0B3B0B060D00030E49133A0B3B0B380B00
-  - Name:.debug_line
-Type:SHT_PROGBITS
-AddressAlign:0x1
-Content: 
3C0004003600010101FB0E0D00010101010001012F746D70676E752D7374796C652D636F6D7072657373696F6E2E63707100
-DWARF:
-  debug_str:
-- clang version 13.0.0
-- '/tmp/gnu-style-compression.cpp'
-- '/tmp/my_working_directory'
-- s
-- short
-- a
-- long int
-- A
-...
Index: lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
===
--- lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
+++ lldb/test/Shell/ObjectFile/ELF/compressed-sections.yaml
@@ -16,6 +16,7 @@
 Type:SHT_PROGBITS
 Flags:   [ SHF_COMPRESSED ]
 Content: deadbeefbaadf00d
+## The legacy .zdebug format is not supported, so the content is not 
uncompressed.
   - Name:.zdebug_info
 Type:SHT_PROGBITS
 Content: 5A4C49420008789c533070084828689809c802c1
@@ -40,5 +41,5 @@
 # CHECK: dwarf-info
 # CHECK: File size: 28
 # CHECK: Data: (
-# CHECK-NEXT: 20304050 60708090
+# CHECK-NEXT: 5A4C4942  0008 789C5330 70084828 6898 09C802C1
 # CHECK-NEXT: )
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -3365,8 +3365,7 @@
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
   size_t result = ObjectFile::ReadSectionData(section, section_data);
-  if (result == 0 || !llvm::object::Decompressor::isCompressedELFSection(
- section->Get(), section->GetName().GetStringRef()))
+  if (result == 0 || !(section->Get() & llvm::ELF::SHF_COMPRESSED))
 return result;
 
   auto Decompressor = llvm::object::Decompressor::create(


Index: lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/gnu-style-compression.yaml
+++ /dev/null
@@ -1,56 +0,0 @@
-## NB: This is a yaml file because llvm gnu-style writing support in 14.0
-## (2022, D117744). In due time we may want to remove it from lldb as well.
-
-## Debug info generated from 

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.



Comment at: llvm/include/llvm/Support/Compression.h:54
+
+void compress(StringRef InputBuffer, SmallVectorImpl ,
+  int Level = DefaultCompression);

I changed the zlib functions to use uint8_t * and ArrayRef instead. 
This patch needs to switch to uint8_t too.



Comment at: llvm/lib/Support/Compression.cpp:155
+bool zstd::isAvailable() { return false; }
+void zstd::compress(StringRef InputBuffer,
+SmallVectorImpl , int Level) {

uint8_t


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The patch looks mostly good but please fix the subject and the summary.

Note: if you fix the local commit message, you can use ` arc diff --head=HEAD 
'HEAD^' --verbatim` to update the subject and the summary.




Comment at: llvm/lib/Support/Compression.cpp:87
  UncompressedSize);
-  UncompressedBuffer.truncate(UncompressedSize);
+  if (UncompressedSize < UncompressedBuffer.size()) {
+UncompressedBuffer.truncate(UncompressedSize);

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

ditto below



Comment at: llvm/lib/Support/Compression.cpp:133
+   size_t ) {
+  size_t const Res =
+  ::ZSTD_decompress((char *)UncompressedBuffer, UncompressedSize,

`const size_t`



Comment at: llvm/unittests/Support/CompressionTest.cpp:69
+#if LLVM_ENABLE_ZSTD
+
+void TestZstdCompression(StringRef Input, int Level) {

delete blank line



Comment at: llvm/unittests/Support/CompressionTest.cpp:70
+
+void TestZstdCompression(StringRef Input, int Level) {
+  SmallString<32> Compressed;

`static void testZstd...`



Comment at: llvm/unittests/Support/CompressionTest.cpp:73
+  SmallString<32> Uncompressed;
+
+  zstd::compress(Input, Compressed, Level);

delete blank line 



Comment at: llvm/unittests/Support/CompressionTest.cpp:99
+  for (size_t i = 0; i < kSize; ++i) {
+BinaryData[i] = i & 255;
+  }

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128465#3646561 , @ckissane wrote:

> In D128465#3646258 , @MaskRay wrote:
>
>> In D128465#3646203 , @ckissane 
>> wrote:
>>
>>> In D128465#3642997 , @MaskRay 
>>> wrote:
>>>
 As I mentioned, the proper approach is to add zstd functionality along 
 with the CMake change, instead of adding CMake to all llvm-project 
 components without a way to test them.
>>>
>>> @MaskRay, I have now done this and ran the ldd tests as requested:
>>>
>>>   With LLVM_ENABLE_ZSTD=ON
>>>   $ ninja check-lld
>>>   Testing Time: 8.98s
>>> Unsupported  :   17
>>> Passed   : 2638
>>> Expectedly Failed:1
>>>   With LLVM_ENABLE_ZSTD=OFF
>>>   $ ninja check-lld
>>>   Testing Time: 8.95s
>>> Unsupported  :   17
>>> Passed   : 2638
>>> Expectedly Failed:1
>>
>> I request that you abandon this patch and incorporate the llvm cmake part 
>> into the llvm patch which you actually use zstd.
>> It is not appropriate to add zstd cmake to llvm-project components which 
>> don't use zstd.
>
> Let me see if I understand this correctly:
> Are you saying that I should abandon this patch, and create a new patch that:
>
> - adds findZSTD.cmake
> - adds zstd to compression namespace
> - adds tests to CompressionTest.cpp
> - and does the **minimal** amount of cmake work to have the flags and test 
> work
>   - thus differing per component cmake config to patches like how you are 
> doing in https://reviews.llvm.org/D129406
>
> Is this correct?
>
> I realize though that then https://reviews.llvm.org/D129406 or similar would 
> be "the [first] llvm patch which actually use[s] zstd"

You can find a component in llvm which uses zstd (e.g. lib/Support). Add the 
functionality with tests (e.g. a CompressionTest.cpp unittest) along with the 
CMake/(optional Bazel, gn) changes.
Keep clang, clang-tools-extra, lld, etc unchanged. These components don't yet 
use zstd and the CMake changes in this patch will be untestable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128465#3646203 , @ckissane wrote:

> In D128465#3642997 , @MaskRay wrote:
>
>> As I mentioned, the proper approach is to add zstd functionality along with 
>> the CMake change, instead of adding CMake to all llvm-project components 
>> without a way to test them.
>
> @MaskRay, I have now done this and ran the ldd tests as requested:
>
>   With LLVM_ENABLE_ZSTD=ON
>   $ ninja check-lld
>   Testing Time: 8.98s
> Unsupported  :   17
> Passed   : 2638
> Expectedly Failed:1
>   With LLVM_ENABLE_ZSTD=OFF
>   $ ninja check-lld
>   Testing Time: 8.95s
> Unsupported  :   17
> Passed   : 2638
> Expectedly Failed:1

I request that you abandon this patch and incorporate the llvm cmake part into 
the llvm patch which you actually use zstd.
It is not appropriate to add zstd cmake to llvm-project components which don't 
use zstd.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

As I mentioned, the proper approach is to add zstd functionality along with the 
CMake change, instead of adding CMake to all llvm-project components without a 
way to test them.




Comment at: lld/test/lit.site.cfg.py.in:21
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@

ckissane wrote:
> ckissane wrote:
> > MaskRay wrote:
> > > MaskRay wrote:
> > > > This needs a change in lld/test/CMakeLists.txt
> > > > 
> > > > Looks like you haven't run `ninja check-lld` as I mentioned.
> > > Actually I am preparing a lld patch and you probably should drop 
> > > have_zstd from lld/ for this patch.
> > I get this when I try to run that:
> > `ninja: error: unknown target 'check-lld', did you mean 'check-lit'?`
> Got it
During cmake, ensure that LLVM_ENABLE_PROJECTS contains `lld`

I posted the patch link to another patch of yours: 
https://reviews.llvm.org/D129406




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

[Fangrui Song via Phabricator via lldb-commits]

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

This patch has changed a lot from what I have reviewed. The CMake change should 
be added along with `llvm::compression::zstd::*` functions.
Otherwise the change just introduces some CMake variables which cannot be 
tested.

Since you haven't touched flang, compiler-rt, etc. The patch should not updated 
their CMake files.

For lld/ELF, I've created D129406  to add the 
support. It will need to wait until you have landed these zstd changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lld/test/lit.site.cfg.py.in:21
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@

MaskRay wrote:
> This needs a change in lld/test/CMakeLists.txt
> 
> Looks like you haven't run `ninja check-lld` as I mentioned.
Actually I am preparing a lld patch and you probably should drop have_zstd from 
lld/ for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lld/test/lit.site.cfg.py.in:21
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@

This needs a change in lld/test/CMakeLists.txt

Looks like you haven't run `ninja check-lld` as I mentioned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

Consider adding `have_zstd` to `compiler-rt/test/lit.common.cfg.py`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added a comment.

Please check both `LLVM_ENABLE_ZSTD={on,off}` work. Ideally, check that when 
zstd' cmake (`libzstd-dev` on Debian) is unavailable, `LLVM_ENABLE_ZSTD=on` 
works (there is no zstd functionality but the cmake invocation should succeed).




Comment at: llvm/cmake/modules/FindZSTD.cmake:3
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.

MaskRay wrote:
> Search `SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception` from other 
> `.cmake` files
Delete `//`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.h:144
+
+StringRef LayoutEndianness = Triple.isLittleEndian() ? "e" : "E";
+

gbenyei wrote:
> MaskRay wrote:
> > You may use a `char` and possibly fold this into the expression below.
> Concatenating a conditional char and a string literal might be tricky, I'm 
> not sure there is a cleaner solution.
There is a constructor `Twine(char)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612

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

[Lldb-commits] [PATCH] D128465: Zstandard as a second compression method to LLVM

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/cmake/modules/FindZSTD.cmake:1
+# Copyright (c) Meta Platforms, Inc. and affiliates.
+#

How did you derive this?

The file seems contributed by you (I don't think facebook/zstd has such a 
file). It should not have a Meta Platforms, Inc notice.



Comment at: llvm/cmake/modules/FindZSTD.cmake:3
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.

Search `SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception` from other 
`.cmake` files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128612#3618167 , @gbenyei wrote:

> In D128612#3617955 , @MaskRay wrote:
>
>> lld/ELF change should be dropped from this change. Don't use 
>> `config->endianness`.
>> I feel sad that for little-endian users who don't use big-endian, every 
>> write now is slightly slower due to a check ;-)
>
> Hi, I'm not sure I get it. How will we have a fully functional toolchain, if 
> I don't implement the lld/ELF part?
> In LLVM, unlike in GCC, target related decisions happen in runtime. I think 
> it's a high level design decision. While I can understand the pain of LE 
> developers getting a slightly slower linker due to endianness checking, I 
> sure will feel the pain of a BE developer not having a linker...
>
> Please explain why I shouldn't use `config->endianness`?

See PPC64.cpp. See D96188  how I added 
aarch64_be support. A set of representative tests should be picked with be 
tests.
If llvm-project consensus is that we will add big-endian support, I can handle 
lld/ELF part. I am mostly concerned with this scenarios that some RISC-V folks 
click LGTM, and the change lands with no test in some areas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612

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

[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

lld/ELF change should be dropped from this change. Don't use 
`config->endianness`.
I feel sad that for little-endian users who don't use big-endian, every write 
now is slightly slower due to a check ;-)




Comment at: clang/lib/Basic/Targets/RISCV.cpp:124
   Builder.defineMacro("__riscv");
-  bool Is64Bit = getTriple().getArch() == llvm::Triple::riscv64;
+  bool Is64Bit = (getTriple().getArch() == llvm::Triple::riscv64 ||
+  getTriple().getArch() == llvm::Triple::riscv64be);

The convention doesn't add `()` in such an assignment.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:220
 
-  if (getTriple().getArch() == llvm::Triple::riscv64) {
+  if (getTriple().getArch() == llvm::Triple::riscv64 ||
+  getTriple().getArch() == llvm::Triple::riscv64be) {

This can be simplified with something like `isRISCV64()`



Comment at: clang/lib/Basic/Targets/RISCV.h:144
+
+StringRef LayoutEndianness = Triple.isLittleEndian() ? "e" : "E";
+

You may use a `char` and possibly fold this into the expression below.



Comment at: clang/lib/Basic/Targets/RISCV.h:145
+StringRef LayoutEndianness = Triple.isLittleEndian() ? "e" : "E";
+
+resetDataLayout(

delete blank line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612

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

[Lldb-commits] [PATCH] D128465: Zstandard as a second compression method to LLVM

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128465#3612948 , @phosek wrote:

> I think this patch should be broken into at least two:
>
> 1. Refactor `llvm/include/llvm/Support/Compression.h` and 
> `llvm/lib/Support/Compression.cpp` to introduce a generic interface and use 
> it throughout the codebase.
> 2. zstd support in `llvm/include/llvm/Support/Compression.h` including the 
> CMake bits.
>
> When uploading future changes, please also make sure to include full context.

Agree. As soon as the namespace refactoring is in a good enough shape, I think 
you may land the refactoring part before the rest of zstd patches.

Note: if you have a deep stacked patches, it may be useful to have a branch 
somewhere (e.g. your llvm-project fork on Github) so that interested folks can 
get the whole picture more easily.
(`arc patch Dx` can technically apply a deep stack, but it often fails to 
apply changes cleanly.)


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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I like the idea of `all` for llvm-objdump, but it should be a separate change, 
with `-mattr=-all` tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127741

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

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D124673#3512190 , @phosek wrote:

> In D124673#3512070 , @MaskRay wrote:
>
>> In D124673#3512037 , @paulkirth 
>> wrote:
>>
>>> Hi, Sorry for the late notification, but I think this change may not apply 
>>> correctly to all configs.
>>>
>>> We're seeing a breakage in Fuchsia's Clang CI builders: 
>>> https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814329895908917697/overview
>>>
>>>   FAILED: lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o 
>>>   /b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/x/w/cipd/bin/clang++ 
>>> --sysroot=/b/s/w/ir/x/w/cipd/linux -DGTEST_HAS_RTTI=0 -D_DEBUG 
>>> -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS 
>>> -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/staging/llvm_build/lib/LineEditor 
>>> -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor 
>>> -I/b/s/w/ir/cache/vpython/79db7c/lib/python3.8/site-packages/tensorflow/include
>>>  -I/b/s/w/ir/x/w/staging/llvm_build/include 
>>> -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/include -isystem 
>>> /b/s/w/ir/x/w/staging/zlib_install/include -stdlib=libc++ -fPIC 
>>> -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time 
>>> -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
>>> -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
>>> -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
>>> -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
>>> -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment 
>>> -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color 
>>> -ffunction-sections -fdata-sections 
>>> -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build=../staging/llvm_build 
>>> -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes 
>>> -O3 -DNDEBUG  -fno-exceptions -fno-unwind-tables 
>>> -fno-asynchronous-unwind-tables -fno-rtti -UNDEBUG -std=c++14 -MD -MT 
>>> lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -MF 
>>> lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o.d -o 
>>> lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -c 
>>> /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp
>>>   /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp:18:10: 
>>> fatal error: 'histedit.h' file not found
>>>   #include 
>>>^~~~
>>>
>>> since the error is a missing header for `histedit.h`, and this change 
>>> alters how libedit is found in CMake, I assume this is the root cause.
>>>
>>> I'm guessing some part of the old CMake checks like the old 
>>> `HAVE_HISTEDIT_H` may not have propagated correctly in all cases?
>>>
>>> If this will be hard to fix, would you mind reverting until a fix is ready?
>>
>> It's not clear to me that this patch caused the issue for you, so I don't 
>> think it is right to ask for a revert now. You need to provide more evidence.
>> lldb-x86_64-debian and lldb-cmake-standalone on 
>> https://lldb.llvm.org/resources/bots.html work well with the change.
>
> I looked a bit more into the error. In the CMake log I see the following:
>
>   -- Found LibEdit: /usr/include (found version ".") 
>
> This is incorrect because in our build we set 
> `CMAKE_SYSROOT=/b/s/w/ir/x/w/cipd/linux` so CMake shouldn't be looking in 
> paths like `/usr/include` because at build time, Clang will be invoked with 
> `--sysroot=/b/s/w/ir/x/w/cipd/linux` and it won't consider include paths like 
> `/usr/include`.
>
> I don't think this issue was introduced in this change, it's a pre-existing 
> issue with the `FindLibEdit.cmake` module, but it was uncovered by this 
> change.

If the patch exposes a preexisting lurking bug, reverting it temporarily is 
fine if that helps you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124673

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

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D124673#3512037 , @paulkirth wrote:

> Hi, Sorry for the late notification, but I think this change may not apply 
> correctly to all configs.
>
> We're seeing a breakage in Fuchsia's Clang CI builders: 
> https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814329895908917697/overview
>
>   FAILED: lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o 
>   /b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/x/w/cipd/bin/clang++ 
> --sysroot=/b/s/w/ir/x/w/cipd/linux -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE 
> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
> -I/b/s/w/ir/x/w/staging/llvm_build/lib/LineEditor 
> -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor 
> -I/b/s/w/ir/cache/vpython/79db7c/lib/python3.8/site-packages/tensorflow/include
>  -I/b/s/w/ir/x/w/staging/llvm_build/include 
> -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/include -isystem 
> /b/s/w/ir/x/w/staging/zlib_install/include -stdlib=libc++ -fPIC 
> -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time 
> -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
> -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
> -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
> -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
> -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion 
> -Wmisleading-indentation -fdiagnostics-color -ffunction-sections 
> -fdata-sections 
> -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build=../staging/llvm_build 
> -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes 
> -O3 -DNDEBUG  -fno-exceptions -fno-unwind-tables 
> -fno-asynchronous-unwind-tables -fno-rtti -UNDEBUG -std=c++14 -MD -MT 
> lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -MF 
> lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o.d -o 
> lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -c 
> /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp
>   /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp:18:10: 
> fatal error: 'histedit.h' file not found
>   #include 
>^~~~
>
> since the error is a missing header for `histedit.h`, and this change alters 
> how libedit is found in CMake, I assume this is the root cause.
>
> I'm guessing some part of the old CMake checks like the old `HAVE_HISTEDIT_H` 
> may not have propagated correctly in all cases?
>
> If this will be hard to fix, would you mind reverting until a fix is ready?

It's not clear to me that this patch caused the issue for you, so I don't think 
it is right to ask for a revert now. You need to provide more evidence.
lldb-x86_64-debian and lldb-cmake-standalone on 
https://lldb.llvm.org/resources/bots.html work well with the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124673

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

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

[Fangrui Song via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb1aed14bfea0: [llvm][lldb] use FindLibEdit.cmake everywhere 
(authored by upsj, committed by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124673

Files:
  cmake/Modules/FindLibEdit.cmake
  lldb/cmake/modules/FindLibEdit.cmake
  lldb/cmake/modules/LLDBStandalone.cmake
  lldb/source/Core/CMakeLists.txt
  lldb/source/Host/CMakeLists.txt
  lldb/source/Interpreter/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/lib/LineEditor/CMakeLists.txt
  llvm/utils/gn/secondary/lldb/source/Host/BUILD.gn

Index: llvm/utils/gn/secondary/lldb/source/Host/BUILD.gn
===
--- llvm/utils/gn/secondary/lldb/source/Host/BUILD.gn
+++ llvm/utils/gn/secondary/lldb/source/Host/BUILD.gn
@@ -142,7 +142,7 @@
   #   list(APPEND EXTRA_LIBS ${CMAKE_DL_LIBS})
   # endif()
   # if (LLDB_ENABLE_LIBEDIT)
-  #   list(APPEND EXTRA_LIBS ${LibEdit_LIBRARIES})
+  #   list(APPEND EXTRA_LIBS LibEdit::LibEdit)
   # endif()
   # if (LLDB_ENABLE_LZMA)
   #   list(APPEND EXTRA_LIBS ${LIBLZMA_LIBRARIES})
Index: llvm/lib/LineEditor/CMakeLists.txt
===
--- llvm/lib/LineEditor/CMakeLists.txt
+++ llvm/lib/LineEditor/CMakeLists.txt
@@ -1,5 +1,5 @@
 if(HAVE_LIBEDIT)
-  set(link_libs edit)
+  set(link_libs LibEdit::LibEdit)
 endif()
 
 add_llvm_component_library(LLVMLineEditor
Index: llvm/cmake/config-ix.cmake
===
--- llvm/cmake/config-ix.cmake
+++ llvm/cmake/config-ix.cmake
@@ -64,7 +64,6 @@
 check_symbol_exists(FE_INEXACT "fenv.h" HAVE_DECL_FE_INEXACT)
 
 check_include_file(mach/mach.h HAVE_MACH_MACH_H)
-check_include_file(histedit.h HAVE_HISTEDIT_H)
 check_include_file(CrashReporterClient.h HAVE_CRASHREPORTERCLIENT_H)
 if(APPLE)
   include(CheckCSourceCompiles)
@@ -184,8 +183,9 @@
   # Don't look for these libraries on Windows.
   if (NOT PURE_WINDOWS)
 # Skip libedit if using ASan as it contains memory leaks.
-if (LLVM_ENABLE_LIBEDIT AND HAVE_HISTEDIT_H AND NOT LLVM_USE_SANITIZER MATCHES ".*Address.*")
-  check_library_exists(edit el_init "" HAVE_LIBEDIT)
+if (LLVM_ENABLE_LIBEDIT AND NOT LLVM_USE_SANITIZER MATCHES ".*Address.*")
+  find_package(LibEdit)
+  set(HAVE_LIBEDIT ${LibEdit_FOUND})
 else()
   set(HAVE_LIBEDIT 0)
 endif()
Index: lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
@@ -10,7 +10,7 @@
 
 
 if (LLDB_ENABLE_LIBEDIT)
-  list(APPEND LLDB_LIBEDIT_LIBS ${LibEdit_LIBRARIES})
+  list(APPEND LLDB_LIBEDIT_LIBS LibEdit::LibEdit)
 endif()
 
 add_lldb_library(lldbPluginScriptInterpreterPython PLUGIN
@@ -35,9 +35,3 @@
   LINK_COMPONENTS
 Support
   )
-
-if (LLDB_ENABLE_LIBEDIT)
-  target_include_directories(lldbPluginScriptInterpreterPython PUBLIC
-${LibEdit_INCLUDE_DIRS}
-  )
-endif()
Index: lldb/source/Interpreter/CMakeLists.txt
===
--- lldb/source/Interpreter/CMakeLists.txt
+++ lldb/source/Interpreter/CMakeLists.txt
@@ -68,6 +68,3 @@
   LLDBInterpreterPropertiesGen
   LLDBInterpreterPropertiesEnumGen)
 
-if (LLDB_ENABLE_LIBEDIT)
-  target_include_directories(lldbInterpreter PRIVATE ${LibEdit_INCLUDE_DIRS})
-endif()
Index: lldb/source/Host/CMakeLists.txt
===
--- lldb/source/Host/CMakeLists.txt
+++ lldb/source/Host/CMakeLists.txt
@@ -141,7 +141,7 @@
   list(APPEND EXTRA_LIBS ${CMAKE_DL_LIBS})
 endif()
 if (LLDB_ENABLE_LIBEDIT)
-  list(APPEND EXTRA_LIBS ${LibEdit_LIBRARIES})
+  list(APPEND EXTRA_LIBS LibEdit::LibEdit)
 endif()
 if (LLDB_ENABLE_LZMA)
   list(APPEND EXTRA_LIBS ${LIBLZMA_LIBRARIES})
@@ -151,7 +151,7 @@
 endif()
 
 if (LLDB_ENABLE_LIBEDIT)
-  list(APPEND LLDB_LIBEDIT_LIBS ${LibEdit_LIBRARIES})
+  list(APPEND LLDB_LIBEDIT_LIBS LibEdit::LibEdit)
   if (LLVM_BUILD_STATIC)
 list(APPEND LLDB_SYSTEM_LIBS gpm)
   endif()
@@ -171,6 +171,3 @@
 Support
   )
 
-if (LLDB_ENABLE_LIBEDIT)
-  target_include_directories(lldbHost PUBLIC ${LibEdit_INCLUDE_DIRS})
-endif()
Index: lldb/source/Core/CMakeLists.txt
===
--- lldb/source/Core/CMakeLists.txt
+++ lldb/source/Core/CMakeLists.txt
@@ -103,10 +103,6 @@
 # TODO: Remove once we have better layering
 set_target_properties(lldbCore PROPERTIES LINK_INTERFACE_MULTIPLICITY 5)
 
-if (LLDB_ENABLE_LIBEDIT)
-  target_include_directories(lldbCore PRIVATE ${LibEdit_INCLUDE_DIRS})
-endif()

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay 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/D124673/new/

https://reviews.llvm.org/D124673

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

[Lldb-commits] [PATCH] D124760: [lldb] Fix ppc64 detection in lldb

[Fangrui Song via Phabricator via lldb-commits]

MaskRay accepted this revision.
MaskRay added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:315
+  uint32_t endian = header.e_ident[EI_DATA];
+  if(endian == ELFDATA2LSB)
+return ArchSpec::eCore_ppc64le_generic;





Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:326
 return riscvVariantFromElfFlags(header);
+  else if (header.e_machine == llvm::ELF::EM_PPC64)
+return ppc64VariantFromElfFlags(header);

Move before EM_RISCV for an alphabetical order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124760

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

[Lldb-commits] [PATCH] D124673: [llvm][lldb] use FindLibEdit.cmake everywhere

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

I vaguely recall that some lldb bots use a stand-alone build.

Say I have a build directory /tmp/RelA with `-DLLVM_ENABLE_PROJECTS=clang`

  cmake -GNinja -S~/llvm-project/lldb -Bout/lldb -DCMAKE_PREFIX_PATH=/tmp/RelA

will print

  CMake Warning at cmake/modules/LLDBConfig.cmake:52 (find_package):


  
By not providing "FindLibEdit.cmake" in CMAKE_MODULE_PATH this project has  


  
asked CMake to find a package configuration file provided by "LibEdit", but 


  
CMake did not find one. 


  



  
Could not find a package configuration file provided by "LibEdit" with any  


  
of the following names: 


  



  
  LibEditConfig.cmake   


  
  libedit-config.cmake  


  



  
Add the installation prefix of "LibEdit" to CMAKE_PREFIX_PATH or set  
"LibEdit_DIR" to a directory containing one of the above files.  If 
   
"LibEdit" provides a separate development package or SDK, be sure it has
been installed.  
  Call Stack (most recent call first):   
cmake/modules/LLDBConfig.cmake:59 (add_optional_dependency) 

CMakeLists.txt:28 (include)  

I wish that https://discourse.llvm.org/t/rfc-stand-alone-build-support/61291/37 
will make stand-alone builds duplicate less code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124673

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