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

[Nikita Popov via Phabricator via lldb-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: StephenFan.

This change looks strictly worse in isolation, the proposal on discourse did 
not reach consensus, and looking at the diagram in 
https://discourse.llvm.org/t/rfc-refactor-triple-related-classes/70410/11 there 
is zero chance that it is ever going to be accepted.


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] D149641: [docs] Hide collaboration and include graphs in doxygen docs

[Nikita Popov via Phabricator via lldb-commits]

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

LGTM, only the inheritance graph is useful, which this preserves if I 
understand correctly (CLASS_GRAPH is still YES).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149641

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

[Lldb-commits] [PATCH] D140800: [OptTable] Precompute OptTable prefixes union table through tablegen

[Nikita Popov via Phabricator via lldb-commits]

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

LGTM

In D140800#4044983 , 
@serge-sans-paille wrote:

> In D140800#4043723 , @nikic wrote:
>
>> Just to check, this isn't going to cause some warning spew about all those 
>> OptTable implementations being non-final?
>
> nope. Why would there be?

Something about missing virtual destructors on a non-final class? Not sure when 
exactly that warning applies.




Comment at: llvm/unittests/Option/OptionParsingTest.cpp:327
 
-TEST(DISABLED_Option, FindNearestFIXME) {
-  TestOptTable T;

serge-sans-paille wrote:
> @nikic: the `DISABLED_` prefix seems to be a gtest convention, see 
> https://github.com/google/googletest/blob/main/docs/advanced.md#temporarily-disabling-tests
Huh, TIL.


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

https://reviews.llvm.org/D140800

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

[Lldb-commits] [PATCH] D140800: [OptTable] Precompute OptTable prefixes union table through tablegen

[Nikita Popov via Phabricator via lldb-commits]

nikic added a comment.

Just to check, this isn't going to cause some warning spew about all those 
OptTable implementations being non-final?




Comment at: llvm/include/llvm/Option/OptTable.h:256
 
+/// Specialization of
+class GenericOptTable : public OptTable {

Comment looks a bit unfinished :)



Comment at: llvm/unittests/Option/OptionParsingTest.cpp:87
+
+template  class DISABLED_OptTableTest : public ::testing::Test {};
+

What's this about?


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

https://reviews.llvm.org/D140800

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

[Lldb-commits] [PATCH] D139881: [clang] Use a StringRef instead of a raw char pointer to store builtin and call information

[Nikita Popov via Phabricator via lldb-commits]

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

LGTM


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

https://reviews.llvm.org/D139881

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

[Lldb-commits] [PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

[Nikita Popov via Phabricator via lldb-commits]

nikic added a comment.

@TimNN Do you have an IR sample for this? DAE is supposed to strip UB-implying 
attributes when converting arguments to poison here: 
https://github.com/llvm/llvm-project/blob/cbe5b2dd914b7ee47bb4d0f67af154a40be4d208/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp#LL291C17-L291C37


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133036

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

[Lldb-commits] [PATCH] D136144: [lldb] Fix m_hwp_regs size for ppc64le

[Nikita Popov via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6f59f302e435: [lldb] Fix m_hwp_regs size for ppc64le 
(PR54520) (authored by nikic).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136144

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h


Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
@@ -122,7 +122,7 @@
 int mode;   // Defines if watchpoint is read/write/access.
   };
 
-  std::array m_hwp_regs;
+  std::array m_hwp_regs;
 
   // 16 is just a maximum value, query hardware for actual watchpoint count
   uint32_t m_max_hwp_supported = 16;


Index: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
===
--- lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
+++ lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
@@ -122,7 +122,7 @@
 int mode;   // Defines if watchpoint is read/write/access.
   };
 
-  std::array m_hwp_regs;
+  std::array m_hwp_regs;
 
   // 16 is just a maximum value, query hardware for actual watchpoint count
   uint32_t m_max_hwp_supported = 16;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

[Nikita Popov via Phabricator via lldb-commits]

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

The default switch has happened, so unblocking this.




Comment at: clang/test/CodeGenOpenCL/overload.cl:23
   generic int *local *genloc;
   generic int *global *genglob;
+  // CHECK-DAG: call spir_func void @_Z3fooPU3AS1iS0_(i32 addrspace(1)* 
noundef {{.*}}, i32 addrspace(1)* noundef {{.*}})

Maybe initialize the relevant variables instead? I'd assume just NULL would 
work here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133036

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

[Lldb-commits] [PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

[Nikita Popov via Phabricator via lldb-commits]

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

I'd like to block this on `-fsanitize-memory-param-retval` (aka msan eager 
checks) being enabled by default. It seems pretty clear that there is a *lot* 
of UB due to uninitialized parameters floating around, so we should at least 
make sure that it is detected in default sanitizer configurations before we 
start exploiting it this aggressively.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133036

___
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

[Nikita Popov via Phabricator via lldb-commits]

nikic added a comment.

In D130689#3690258 , @thieta wrote:

> In D130689#3689157 , @thakis wrote:
>
>> Is it expected and intentional that this increases the mac deployment target 
>> to 10.12?
>
> I wasn't aware of that - but I think it's expected since the check in RWMutex 
> checks for the C++ standard and doesn't care about the deployment target for 
> macOS. It seems pretty easy to change, but I wonder if that matters? 10.12 
> was released in 2016 so it's pretty old and this wouldn't affect most users 
> of LLVM.
>
> My gut feeling say that we should be fine with requiring 10.12 and if that 
> becomes a big problem during the development phase someone could propose a 
> patch to improve the check in RWMutex.
>
> But in that case we should probably have a check for the deployment target as 
> part of the cmake config and error if CXX_STANDARD > 17 and 
> OSX_DEPLOYMENT_TARGET < 10.12.

Given 
https://github.com/llvm/llvm-project/blob/2bb7c54621f31a957302a4deb3d25b752acb07bd/llvm/include/llvm/Support/RWMutex.h#L22-L27,
 it seems like this is supposed to be supported. This is probably just a matter 
of moving the shared_mutex use behind the LLVM_USE_RW_MUTEX_IMPL guard?


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] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Nikita Popov via Phabricator via lldb-commits]

nikic added a comment.

Given that LLVM 15 branches off in one week, maybe it would be better to wait 
for that before relanding the change, as it seems to have significant impact on 
plugins?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[Lldb-commits] [PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Nikita Popov via Phabricator via lldb-commits]

nikic added a comment.

FYI this change had a noticeable compile-time impact 
(http://llvm-compile-time-tracker.com/compare.php?from=ee88c0cf09969ba44307068797e12533b94768a6=bdc6974f92304f4ed542241b9b89ba58ba6b20aa=instructions),
 is that expected? Largest regressions are in kimwitu++, where error.cc and 
gutil.cc regress by 2%.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[Lldb-commits] [PATCH] D123957: Update the developer policy to mention release notes

[Nikita Popov via Phabricator via lldb-commits]

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



Comment at: llvm/docs/DeveloperPolicy.rst:195
+* Adding, removing, or regrouping a diagnostic.
+* Fixing a bug (please link to the issue fixed in the bug database).
+* Adding or removing an optimization.

aaron.ballman wrote:
> lattner wrote:
> > aaron.ballman wrote:
> > > nikic wrote:
> > > > I disagree with this point. Bug fixes should not make it into the 
> > > > release notes as a matter of course -- there may be specific 
> > > > high-impact bug fixes that may be worth mentioning, but bug fixes 
> > > > should not be included in the release notes as a matter of policy.
> > > > 
> > > > I agree that release notes for Clang/LLVM are currently insufficient, 
> > > > but we should also be careful not to over-compensate in the other 
> > > > direction, but including too much irrelevant noise.
> > > > I disagree with this point. Bug fixes should not make it into the 
> > > > release notes as a matter of course -- there may be specific 
> > > > high-impact bug fixes that may be worth mentioning, but bug fixes 
> > > > should not be included in the release notes as a matter of policy.
> > > 
> > > I disagree (quite strongly) with this and would point out that users have 
> > > explicitly requested this information be included: 
> > > https://discourse.llvm.org/t/rfc-update-developer-policy-on-release-notes/61856/3
> > > 
> > > > I agree that release notes for Clang/LLVM are currently insufficient, 
> > > > but we should also be careful not to over-compensate in the other 
> > > > direction, but including too much irrelevant noise.
> > > 
> > > Bug fixes are generally far from irrelevant, but I agree that reviewer 
> > > and author discretion is advised (as with any release note). For example, 
> > > if you introduce a bug in version N and fix the bug in version N, there's 
> > > no need for a release note in that situation because there's no 
> > > user-facing change as far as users care about. But I don't want to try to 
> > > spell that out in precise detail because there will always be edge cases.
> > I agree with both of you - listing every bug report would be too noisy and 
> > pretty irrelevant for most users, but there are high impact ones that are 
> > important and valuable to list.  How about wording this bullet something 
> > like:
> > 
> > * Fixing high impact bugs, e.g. ones that get discussed on hackernews or 
> > comparable forums (please link to the issue fixed in the bug database).
> Thanks for clarifying where I was misunderstanding before. I agree there 
> needs to be a change to my wording, but I'd prefer if we went with something 
> more like:
> 
> * Fixing a bug that potentially has significant user-facing impact (please 
> link to the issue fixed in the bug database).
> 
> (I mostly want to avoid making it sound like the bug has to be a 
> stop-the-world bug in order to warrant a release note. Functionally, I think 
> fixing a bug in Clang should almost always have a release note, but the same 
> may not be true for fixing a bug in LLVM where the difference in behavior may 
> not be particularly observable to users.)
> 
> Would this address your concerns @nikic?
The new wording looks good to me, thanks!


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

https://reviews.llvm.org/D123957

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

[Lldb-commits] [PATCH] D123957: Update the developer policy to mention release notes

[Nikita Popov via Phabricator via lldb-commits]

nikic requested changes to this revision.
nikic added inline comments.
This revision now requires changes to proceed.



Comment at: llvm/docs/DeveloperPolicy.rst:195
+* Adding, removing, or regrouping a diagnostic.
+* Fixing a bug (please link to the issue fixed in the bug database).
+* Adding or removing an optimization.

I disagree with this point. Bug fixes should not make it into the release notes 
as a matter of course -- there may be specific high-impact bug fixes that may 
be worth mentioning, but bug fixes should not be included in the release notes 
as a matter of policy.

I agree that release notes for Clang/LLVM are currently insufficient, but we 
should also be careful not to over-compensate in the other direction, but 
including too much irrelevant noise.


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

https://reviews.llvm.org/D123957

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