This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove nested switches from ARMGetSupportedArchitectureAtIndex (NFC)
ClosedPublic

Authored by JDevlieghere on Nov 3 2021, 5:33 PM.

Details

Summary

Remove the nested switches from the ARMGetSupportedArchitectureAtIndex implementation. I considered a few alternatives, like changing the API (too intrusive) and trying to be clever by manipulating indexes (too hard to maintain). I'm not happy about the remaining amount of duplication, but it seemed like the best tradeoff.

Diff Detail

Event Timeline

JDevlieghere created this revision.Nov 3 2021, 5:33 PM
JDevlieghere requested review of this revision.Nov 3 2021, 5:33 PM
JDevlieghere added inline comments.Nov 3 2021, 5:36 PM
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
573

I could put this into a def file if desirable, but right now I'm still undecided whether that's overkill.

jasonmolenda accepted this revision.Nov 3 2021, 11:04 PM

Nice cleanup of this table that's been growing over the years.

This revision is now accepted and ready to land.Nov 3 2021, 11:04 PM
labath added a subscriber: labath.Nov 3 2021, 11:35 PM
labath added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
558

Is this any different from an ArrayRef<const char *>?

564–565

This wouldn't be needed with arrayref, as it has an appropriate C array constructor

JDevlieghere marked 2 inline comments as done.Nov 5 2021, 8:20 PM
JDevlieghere added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
558

Totally forgot about that. Thanks for the suggestion.

Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2021, 9:12 PM

Added some follow up comments

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
561

This will default to the ArchSpec::eCore_arm_arm64 case if no matching ArchSpec is specified. Double-checking if this by intent.

640

Could we delete this unreachable return statement? It's tripping up static code checkers since all possible branches already have a return.

Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 1:41 AM
JDevlieghere marked 2 inline comments as done.Aug 5 2022, 11:15 AM
JDevlieghere added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
561

I'm sure it was intentional when I wrote it, but I can't figure out for which core this would actually make sense. If the test suite passes with returning an empty array here, that works for me.

640

Yes, let's make it an llvm_unreachable (otherwise gcc will complain).

Added comments

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
640

If we return {} in the default case, and delete this, gcc should not complain since all possible code paths will have a return, is that right?
Alternatively, we could break in the default case and leave this return alone. I prefer the first approach for consistency.

JDevlieghere marked 3 inline comments as done.Aug 5 2022, 12:24 PM
JDevlieghere added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
640

Yes, you're right, it's only a problem for gcc when you explicitly handle every case (see all the instances of llvm_unreachable("Fully covered switch!"); in lldb/llvm).

fixathon added inline comments.Aug 5 2022, 8:56 PM
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
640

I'll just add a break to the default case to avoid llvm_unreachable(). What's the best way to introduce this change? Can I link it to this diff somehow or a stand-alone new diff?

Added a comment about a failing test.

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
561

When this is fixed, one test is failing, and I think that might be a problem with the test itself: test_ios in test/API/functionalities/gdb_remote_client/TestPlatformMacOSX.py running on MacOS.

File "lvm-project/lldb/test/API/functionalities/gdb_remote_client/TestPlatformMacOSX.py", line 48, in test_ios
  self.platform_test(host="ios",
File "llvm-project/lldb/test/API/functionalities/gdb_remote_client/TestPlatformMacOSX.py", line 44, in platform_test
  self.assertEqual(platform.GetName(), expected_platform)

AssertionError: 'host' != 'remote-ios'

  • host+ remote-ios
clayborg added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
561

Looks like it might be a problem with the test indeed. The "def qHostInfo(self):" from line 21 is returning a CPU type for the host of "cputype:16777223". This is 0x01000007 which is "x86_64" as the 'qHostInfo' packet uses a decimal encoding. Later when asked about the process with "def qProcessInfo(self):" it returns "cputype:100000c" which is hex encoded with no "0x" prefix and it means "arm64".

Not sure if this test is trying to test a remote ios debugging scenario from a desktop machine debugging to a ios device, but if this is the case, both qHostInfo and qProcessInfo should be agreeing on the architecture and using the same thing, which would be "arm64"? These are packets that are usually going to the "debugserver", which is run on a phone. So my guess would be to try and set qHostInfo to have "cputype:16777228" (which is decimal for 0x0100000c) and see if that works.

Jonas, does this make sense?

JDevlieghere marked an inline comment as done.Aug 11 2022, 4:37 PM
JDevlieghere added inline comments.
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
561

No, this is intentional and pretty much the point of this test. There's more details in https://reviews.llvm.org/D121444 but the TL;DR is that this is testing running iPhone and iPad Apps on Apple Silicon Macs. That also explains the default then, which I was pretty sure I added for a reason :-)

clayborg added inline comments.Aug 11 2022, 6:09 PM
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
561

Gotcha, looks like Slava need to debug this and see why we are getting a invalid core type passed to the GetCompatibleArchs() function in the first place.