[lldb][sbdebugger] Move SBDebugger Broadcast bit enum into lldb-enumerations.h #87409

chelcassanova · 2024-04-02T20:29:52Z

When the eBroadcastBitProgressCategory bit was originally added to Debugger.h and SBDebugger.h, each corresponding bit was added in order of the other bits that were previously there. Since Debugger.h has an enum bit that SBDebugger.h does not, this meant that their offsets did not match.

Instead of trying to keep the bit offsets in sync between the two, it's preferable to just move SBDebugger's enum into the main enumerations header and use the bits from there.

github-actions · 2024-04-02T20:33:53Z

✅ With the latest revision this PR passed the C/C++ code formatter.

JDevlieghere · 2024-04-02T21:50:57Z

lldb/include/lldb/API/SBDebugger.h

@@ -46,7 +46,8 @@ class LLDB_API SBDebugger {
      eBroadcastBitProgress = (1 << 0),
      eBroadcastBitWarning = (1 << 1),
      eBroadcastBitError = (1 << 2),
-      eBroadcastBitProgressCategory = (1 << 3),
+      eBroadcastBitProgressCategory =
+          (1 << 4), // This needs to match the corresponding bit in Debugger.h


Let's move that comment above the enum.

JDevlieghere · 2024-04-02T22:12:02Z

lldb/include/lldb/API/SBDebugger.h

@@ -46,7 +46,8 @@ class LLDB_API SBDebugger {
      eBroadcastBitProgress = (1 << 0),
      eBroadcastBitWarning = (1 << 1),
      eBroadcastBitError = (1 << 2),
-      eBroadcastBitProgressCategory = (1 << 3),
+      // This needs to match the corresponding bit in Debugger.h


Doesn't this comment apply to the whole enum? My suggestion was to move this above line 45 to convey that, unless that's not the case?

My mistake, I was indicating that the progress category bit is skipping (1 << 3) in this sequence since that bit has to match the one in Debugger.h. I can change the comment to say that the whole enum needs to match Debugger.h.

When the `eBroadcastBitProgressCategory` bit was originally added to Debugger.h and SBDebugger.h, each corresponding bit was added in order of the other bits that were previously there. Since `Debugger.h` has an enum bit that `SBDebugger.h` does not, this meant that their offsets did not match. This commit changes it so that the bit offsets match each other.

llvmbot · 2024-04-03T20:19:35Z

@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

Changes

When the eBroadcastBitProgressCategory bit was originally added to Debugger.h and SBDebugger.h, each corresponding bit was added in order of the other bits that were previously there. Since Debugger.h has an enum bit that SBDebugger.h does not, this meant that their offsets did not match. This commit changes it so that the bit offsets match each other.

Full diff: https://github.com/llvm/llvm-project/pull/87409.diff

1 Files Affected:

(modified) lldb/include/lldb/API/SBDebugger.h (+3-1)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 62b2f91f5076d5..a994874dde6d4a 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -42,11 +42,13 @@ class LLDB_API SBInputReader {
 
 class LLDB_API SBDebugger {
 public:
+  // The enum values here need to match their corresponding values in
+  // Debugger.h.
   FLAGS_ANONYMOUS_ENUM(){
       eBroadcastBitProgress = (1 << 0),
       eBroadcastBitWarning = (1 << 1),
       eBroadcastBitError = (1 << 2),
-      eBroadcastBitProgressCategory = (1 << 3),
+      eBroadcastBitProgressCategory = (1 << 4),
   };
 
   SBDebugger();

medismailben · 2024-04-05T07:59:29Z

Why don't we just remove the enum from SBDebugger and use the one from Debugger instead, this will ensure the offsets are always the same.

bulbazord · 2024-04-05T20:31:49Z

Why don't we just remove the enum from SBDebugger and use the one from Debugger instead, this will ensure the offsets are always the same.

SBDebugger is public and Debugger is private, so I'm not sure how you can do this? Maybe I'm misunderstanding your suggestion...

It would be nice if we could move the enum into lldb-enumerations so we didn't have to worry about keeping this in sync though.

Per Alex's suggestions, adds the broadcast bits from SBDebugger into lldb-enumerations.

JDevlieghere · 2024-04-10T16:30:15Z

lldb/include/lldb/API/SBDebugger.h

+  // The enum values here need to match their corresponding values in
+  // Debugger.h.
  FLAGS_ANONYMOUS_ENUM(){
      eBroadcastBitProgress = (1 << 0),
      eBroadcastBitWarning = (1 << 1),
      eBroadcastBitError = (1 << 2),
-      eBroadcastBitProgressCategory = (1 << 3),
+      eBroadcastBitProgressCategory = (1 << 4),


This one can go now, right?

Yup, will remove.

JDevlieghere · 2024-04-10T16:30:38Z

lldb/include/lldb/lldb-enumerations.h

@@ -1339,6 +1339,14 @@ enum AddressMaskRange {
  eAddressMaskRangeAll = eAddressMaskRangeAny,
 };

+/// Used by the debugger to indicate which events are being broadcasted.
+enum DebuggerBroadcast {


DebuggerBroadcastBit maybe?

medismailben

LGTM if you address the comment and rewrite the PR description and title :)

medismailben · 2024-04-10T21:09:54Z

lldb/include/lldb/lldb-enumerations.h

+  eBroadcastBitProgress = (1 << 0),
+  eBroadcastBitWarning = (1 << 1),
+  eBroadcastBitError = (1 << 2),
+  eBroadcastBitProgressCategory = (1 << 4),


typo ?

Suggested change

eBroadcastBitProgressCategory = (1 << 4),

eBroadcastBitProgressCategory = (1 << 3),

Oh yeah, I guess since this is in lldb-enumerations it doesn't have to match the ones in Debugger.h anymore.

Removes the enum for broadcast bits from SBDebugger as they're now in lldb-enumerations.h. This also requires uses of these bits to be updated in some API tests.

…db-enumerations.h" (#88324) Reverts #87409 due a missed update to the broadcast bit causing a build failure on the x86_64 Debian buildbot.

llvm#87409 removed the broadcast bits from SBDebugger and placed them in `lldb-enumerations.h`. This is API-breaking so this commits places the enum back into `SBDebugger.h` and references the bits from `lldb-enumerations.h`.

#87409 removed the broadcast bits from SBDebugger and placed them in `lldb-enumerations.h`. This is API-breaking so this commits places the enum back into `SBDebugger.h` and references the bits from `lldb-enumerations.h`. rdar://127128536

llvm#87409 removed the broadcast bits from SBDebugger and placed them in `lldb-enumerations.h`. This is API-breaking so this commits places the enum back into `SBDebugger.h` and references the bits from `lldb-enumerations.h`. rdar://127128536 (cherry picked from commit a4c21d1)

chelcassanova requested a review from JDevlieghere as a code owner April 2, 2024 20:29

chelcassanova force-pushed the match-debugger-progress-category-bits branch from efa26c8 to 88c6b49 Compare April 2, 2024 20:30

JDevlieghere reviewed Apr 2, 2024

View reviewed changes

chelcassanova force-pushed the match-debugger-progress-category-bits branch from 88c6b49 to b3ff6c7 Compare April 2, 2024 21:56

JDevlieghere reviewed Apr 2, 2024

View reviewed changes

chelcassanova force-pushed the match-debugger-progress-category-bits branch from b3ff6c7 to 7561d03 Compare April 3, 2024 20:19

llvmbot added the lldb label Apr 3, 2024

JDevlieghere approved these changes Apr 4, 2024

View reviewed changes

Add SBDebugger broadcast bits to lldb-enumerations

a37c65d

Per Alex's suggestions, adds the broadcast bits from SBDebugger into lldb-enumerations.

JDevlieghere reviewed Apr 10, 2024

View reviewed changes

medismailben self-requested a review April 10, 2024 21:08

medismailben approved these changes Apr 10, 2024

View reviewed changes

Remove broadcast enum from SBDebugger

39b05ff

Removes the enum for broadcast bits from SBDebugger as they're now in lldb-enumerations.h. This also requires uses of these bits to be updated in some API tests.

chelcassanova force-pushed the match-debugger-progress-category-bits branch from 3dc9c9f to 39b05ff Compare April 10, 2024 21:37

chelcassanova changed the title ~~[lldb][sbdebugger] Match progress category enum bit in Debugger.h~~ [lldb][sbdebugger] Move SBDebugger Broadcast bit enum into lldb-enumerations.h Apr 10, 2024

chelcassanova merged commit af7c196 into llvm:main Apr 10, 2024
4 checks passed

chelcassanova deleted the match-debugger-progress-category-bits branch April 10, 2024 21:45

chelcassanova mentioned this pull request Apr 10, 2024

Revert "[lldb][sbdebugger] Move SBDebugger Broadcast bit enum into lldb-enumerations.h" #88324

Merged

chelcassanova mentioned this pull request Apr 26, 2024

[lldb][sbapi] Fix API break in SBDebugger broadcast bits #90261

Merged

chelcassanova mentioned this pull request Apr 26, 2024

[lldb][sbapi] Fix API break in SBDebugger broadcast bits (#90261) swiftlang/llvm-project#8645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb][sbdebugger] Move SBDebugger Broadcast bit enum into lldb-enumerations.h #87409

[lldb][sbdebugger] Move SBDebugger Broadcast bit enum into lldb-enumerations.h #87409

chelcassanova commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

JDevlieghere Apr 2, 2024

JDevlieghere Apr 2, 2024

chelcassanova Apr 2, 2024

llvmbot commented Apr 3, 2024

medismailben commented Apr 5, 2024

bulbazord commented Apr 5, 2024

JDevlieghere Apr 10, 2024

chelcassanova Apr 10, 2024

JDevlieghere Apr 10, 2024

medismailben left a comment

medismailben Apr 10, 2024

chelcassanova Apr 10, 2024

	eBroadcastBitProgressCategory = (1 << 4),
	eBroadcastBitProgressCategory = (1 << 3),

[lldb][sbdebugger] Move SBDebugger Broadcast bit enum into lldb-enumerations.h #87409

[lldb][sbdebugger] Move SBDebugger Broadcast bit enum into lldb-enumerations.h #87409

Conversation

chelcassanova commented Apr 2, 2024 • edited Loading

github-actions bot commented Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Apr 3, 2024

medismailben commented Apr 5, 2024

bulbazord commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medismailben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chelcassanova commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading