[LLDB] Enable 64 bit debug/type offset

[ayermolo]

Summary

This came out of from https://discourse.llvm.org/t/dwarf-dwp-4gb-limit/63902
With big binaries we can have .dwp files where .debug_info.dwo section can grow
beyond 4GB. We would like to support this in LLVM and in LLDB.

The plan is to enable manual parsing of cu/tu index in DWARF library
(https://reviews.llvm.org/D137882), and then
switch internal index data structure to 64 bit.
For the second part is to enable 64bit offset support in LLDB with
this patch.

[clayborg]

In D138618#4080706, @labath wrote:

In D138618#4077851, @ayermolo wrote:

In D138618#4073329, @labath wrote:

I think that the 1-based index thingy helps a lot here, but I haven't seen anything (in your reponse, or in the new patch) that would address my concernt DIERef<->user_id conversion ambiguity. I.e. how is one supposed to know what is the "right" way to convert a DIERef to a user_id:

• die_ref.get_id()
• or symbol_file.GetUID(die_ref) (which, funnily enough, will construct another DIERef, and *then* call get_id? (return DIERef(GetID(), ref.section(), ref.die_offset()).get_id();)

What's your position on that? That we should live with the ambiguity?

Searching for GetUID doesn't look like it's used all that often, maybe follow up patch is just to get rid of it, and replace with DIERef?

If you could make that work, that would be awesome, but I think that's going to be fairly hard. It's true that there aren't that many call sites of this functions, but the ones that are there are very crucial. The user_id_t type represents a symbol-file-neutral identifier (cookie, if you will) that different symbol file implementations use to identify parsed objects (types, mostly). SymbolFileDWARF uses it (via DIERef et al.) to identify the DIE belonging to that type. PDB symbol files use it differently, but the idea is the same. If we wanted to remove that, we'd have to come up with a whole new way to parse/link types -- and one that would work for non-dwarf symbol files as well.

SymbolFileDWARF is the only SymbolFile that inherits from UserID. So this is a DWARF internal thing only where symbol files have user_id_t values. So as long as we make this work for all things DWARF we are good to go.

(also, this would not really address my concern, because the question would then become "which DIERef is safe to be used as a Type cookie" (answer: only the one which has the OSO field set), but the reduction in complexity resulting from removing one step from the conversion process (DWARFDIE->DIERef->user_id) might be well worth it.)

DIERef should be used anywhere a DIE needs a user_id_t. If DIERef now uses the file index (OSO index, or DWO index) in the same way where we always ask the SymbolFileDWARFXXX::GetID() as the file index, then things will be the same between the two (DWO or OSO).

[clayborg]

We just need to create all DIERef objects using the GetID() from the symbol file as the file index, and we should be able to remove the SymbolFile::GetUID() function now. As long as file index zero is reserved for "vanilla DWARF that doesn't use DWO or OSO we will know the difference. We might want to not have SymbolFileDWARF inherit from UserID at all, and switch over to have SymbolFileDWARF add a virtual function:

uint32_t m_file_index = 0; // Zero means main DWARF file, 1...N identifies the Nth DWO file or OSO file
virtual uint32_t GetFileIndex() { return m_file_index; }

Then anyone can set the file index correctly for DWO or OSO files. And we avoid using user_id_t values for the symbol files since they aren't needed.

lldb/source/Plugins/SymbolFile/DWARF/DIERef.h

72

We should be able to, and we should move this lldbassert to the constructor to ensure that die_offset is not too large.

[labath]

In D138618#4083481, @clayborg wrote:

We just need to create all DIERef objects using the GetID() from the symbol file as the file index, and we should be able to remove the SymbolFile::GetUID() function now. As long as file index zero is reserved for "vanilla DWARF that doesn't use DWO or OSO we will know the difference. We might want to not have SymbolFileDWARF inherit from UserID at all, and switch over to have SymbolFileDWARF add a virtual function:

uint32_t m_file_index = 0; // Zero means main DWARF file, 1...N identifies the Nth DWO file or OSO file
virtual uint32_t GetFileIndex() { return m_file_index; }

Then anyone can set the file index correctly for DWO or OSO files. And we avoid using user_id_t values for the symbol files since they aren't needed.

This isn't about the "user id" of a symbol file. I'm totally happy with the changes there -- though I also wouldn't be opposed to changing the "user id" field to something more explicit (like the file index).

My problem is with the "user id"s of individual DIEs. Currently, if I have a DWARFDIE, the only way to get its user id is to do something like die.GetDWARF()->GetUID(die). With this patch, there are two ways:

1. the same as before
2. die.GetDIERef()->get_id()

The problem is that the second way is not going to be correct for OSO files because that path will not set the "oso" component of the DIERef. The worst part is that the second method is much shorter than the first one, so I think it will be very tempting to use it -- and it will actually be right most of the time, until that code is used in an OSO context.

[ayermolo]

addressed comments

[ayermolo]

Created commit that removes getUID(...) https://reviews.llvm.org/D142775
Seems like it's isolated to SymbolFile and DWARF code.
So now userid goes through DIERef.

[labath]

I'm sorry, but that patch does not fix the problem I am trying to point out. In fact, I think it makes things a lot worse.

We clearly have some kind of a communication problem, but I am running out of ideas of what can I do about it. Let me try rephrasing it one more time:

• this patch creates two path for converting a DIERef to a user_id_t -- a) ref.get_id(); and b) dwarf.GetUID(ref)
• of those two ways, one is clearly more intuitive
• of those two ways, one is always correct
• those two ways aren't the same -- (a) is simpler; (b) is correct
• you can't fix that by simply taking (b) away. All that does is make the API misuse even more likely. That patch essentially just deletes GetUID, and inlines it into all its callers.

Forget about the what the code does for a moment, and tell me which of these snippets looks better:
i)

if (IsValid())
  return GetDWARF()->GetUID(*this);

ii)

const std::optional<DIERef> &ref = this->GetDIERef();
if (ref)
  return DIERef(GetID(), ref->section(), ref->die_offset()).get_id();

iii)

if (IsValid())
  return GetDIERef()->get_id();

Now look up the implementation and tell me which one is correct.

[ayermolo]

In D138618#4086789, @labath wrote:

I'm sorry, but that patch does not fix the problem I am trying to point out. In fact, I think it makes things a lot worse.

We clearly have some kind of a communication problem, but I am running out of ideas of what can I do about it. Let me try rephrasing it one more time:

• this patch creates two path for converting a DIERef to a user_id_t -- a) ref.get_id(); and b) dwarf.GetUID(ref)
• of those two ways, one is clearly more intuitive
• of those two ways, one is always correct
• those two ways aren't the same -- (a) is simpler; (b) is correct
• you can't fix that by simply taking (b) away. All that does is make the API misuse even more likely. That patch essentially just deletes GetUID, and inlines it into all its callers.

Forget about the what the code does for a moment, and tell me which of these snippets looks better:
i)

if (IsValid())
  return GetDWARF()->GetUID(*this);

ii)

const std::optional<DIERef> &ref = this->GetDIERef();
if (ref)
  return DIERef(GetID(), ref->section(), ref->die_offset()).get_id();

iii)

if (IsValid())
  return GetDIERef()->get_id();

Now look up the implementation and tell me which one is correct.

Thank you for providing an example. Yes sometimes it's hard to communicate over comments.
In this context yes first one is better.
Question is what should it look "under the hood".
For example:
DIERef::Decode
SymbolFileDWARF::GetUID
SymbolFileDWARF::DecodeUID
There are all these bit shifts scattered around.

If this is such a blocker I did expand on your diff, and added 64 bit support (it still has to be cleaned up a bit like various static constexpr probably moved out of DIERef to #define in dwarfh, but for illustrative purposes) https://reviews.llvm.org/D142779

[clayborg]

Looks good to me. Pavel?

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

67

Why is this needed? No casting should be needed for using the llvm formatting stuff?

198

Needed? Same as above

[clayborg]

In D138618#4086789, @labath wrote:

I'm sorry, but that patch does not fix the problem I am trying to point out. In fact, I think it makes things a lot worse.

We clearly have some kind of a communication problem, but I am running out of ideas of what can I do about it. Let me try rephrasing it one more time:

• this patch creates two path for converting a DIERef to a user_id_t -- a) ref.get_id(); and b) dwarf.GetUID(ref)
• of those two ways, one is clearly more intuitive
• of those two ways, one is always correct
• those two ways aren't the same -- (a) is simpler; (b) is correct
• you can't fix that by simply taking (b) away. All that does is make the API misuse even more likely. That patch essentially just deletes GetUID, and inlines it into all its callers.

Forget about the what the code does for a moment, and tell me which of these snippets looks better:
i)

if (IsValid())
  return GetDWARF()->GetUID(*this);

ii)

const std::optional<DIERef> &ref = this->GetDIERef();
if (ref)
  return DIERef(GetID(), ref->section(), ref->die_offset()).get_id();

iii)

if (IsValid())
  return GetDIERef()->get_id();

Now look up the implementation and tell me which one is correct.

Sorry a lot of noise on all of these things. Forget me last comment, I hit submit too quickly.

How about we make DIERef constructor always take all the info that is needed to construct the objects correctly:

DIERef(DWARFDie die);
DIERef(SymbolFileDWARF *dwarf, dw_offset_t die_offset); // might not need this one?
DIERef(user_id_t uid);

We might not need all of these. But in this case, we can't incorrectly use the APIs since all of the objects that are needed to fill it in are in the constructor args. We take away the ability to manually fill in the DWO num and other fields. Would that fix the issues you have with this patch Pavel?

[clayborg]

So looks like this function needs to be fixed:

llvm::Optional<DIERef> DWARFBaseDIE::GetDIERef() const {
  if (!IsValid())
    return llvm::None;

  return DIERef(m_cu->GetSymbolFileDWARF().GetDwoNum(), m_cu->GetDebugSection(),
                m_die->GetOffset());
}

This currently only works for DWO files, but not for OSO files. If we make the DIERef constructor that this function uses "protected", then only this function can use it. And it should be able to do things correctly.

If we switched the DIERef constructor to take a SymbolFileDWARF as a mandatory first arg, then we can manually supply the section + offset, then DWARFBaseDie can always do this correctly for both DWO files and OSO files.

We might need to add an accessor to SymbolFileDWARF like:

virtual uint32_t SymbolFileDWARF::GetFileIndex();

And there can be a setter for this as well. Then the OSO stuff would set the file index to a 1 based index, and the DWO files would also set this as the file index in the SymoblFileDWARF base class and then this all works.

So my suggestion would be:

• make only one DIERef constructor: "DIERef(SymbolFileDWARF *dwarf, Section section, dw_offset_t die_offset)"
• add SymbolFileDWARF::GetFileIndex() and SymbolFileDWARF::SetFileIndex(...) and a backing ivar
• OSO and DWO files will set the file index manually early on so it is always available and can always create valid DIERef objects in DWARFBaseDIE::GetDIERef
• Change DWARFBaseDIE::GetDIERef to use the GetFileIndex() where it expects zero for a non DWO or OSO file and a 1 based index for DWO or OSO stuff
• Don't allow anyone else to create manually DIERef objects unless you ask for it from a DWARFBaseDie except for the encode/decode functions for saving to/from cache

[labath]

In D138618#4092040, @clayborg wrote:

How about we make DIERef constructor always take all the info that is needed to construct the objects correctly:

DIERef(DWARFDie die);
DIERef(SymbolFileDWARF *dwarf, dw_offset_t die_offset); // might not need this one?
DIERef(user_id_t uid);

We might not need all of these. But in this case, we can't incorrectly use the APIs since all of the objects that are needed to fill it in are in the constructor args. We take away the ability to manually fill in the DWO num and other fields. Would that fix the issues you have with this patch Pavel?

Yes, I believe it would, but I do want to add two things:

• I don't consider it important whether most of the construction work happens inside the DIERef constructor, or outside of it. So, I would consider these two implementations equally fine

DIERef(DWARFDie die); // compute this somehow
DWARFDie::GetDIERef() { return DIERef(*this); }

vs.

DIERef(dwo_id, type_unit_flag, die_offset, ...); // a dumb constructor
DWARFDie::GetDIERef() { return DIERef(...); } // computation happens here

The first one is what you described -- the second one is how it roughly how it works right now.

• My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.

[clayborg]

• My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.

I do want to state that if we fix things the way I describe it will work seamlessly with OSO or DWO files. The current state of things is the DWO stuff only uses the fancy DIERef constructor and fills in the dwo number correctly only to have it overwritten in SymbolFile::GetUID(...). The SymbolFile::GetUID(...) is needed for OSOs currently because the DIERef that SymbolFileDWARF (which is used for OSO) doesn't correctly create DIERef objects since they always return llvm::None for SymbolFileDWARF::GetDwoNum(). But the new API will have SymbolFileDWARF::GetFileIndex() to be used instead of SymbolFileDWARF::GetDwoNum(), and the file index will be set correctly for both DWO and OSO files. We can then change DIERef away from DWO specific naming, and have DIERef have a "m_file_index" and "m_file_index_valid" instead of the dwo specific members. As long as both OSO and DWO files can be found from the user_id_t API calls we are all good. Not sure if this addresses all of your issues or not.

If all of your concerns are not clarified above, can you clarify what is still being overlooked? Both Alexander and I are usually thinking we are addressing everything you want, but we obviously still aren't, so restating your remaining concerns would help us get this patch moving.

[labath]

In D138618#4094933, @clayborg wrote:

• My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.

I do want to state that if we fix things the way I describe it will work seamlessly with OSO or DWO files. The current state of things is the DWO stuff only uses the fancy DIERef constructor and fills in the dwo number correctly only to have it overwritten in SymbolFile::GetUID(...). The SymbolFile::GetUID(...) is needed for OSOs currently because the DIERef that SymbolFileDWARF (which is used for OSO) doesn't correctly create DIERef objects since they always return llvm::None for SymbolFileDWARF::GetDwoNum(). But the new API will have SymbolFileDWARF::GetFileIndex() to be used instead of SymbolFileDWARF::GetDwoNum(), and the file index will be set correctly for both DWO and OSO files. We can then change DIERef away from DWO specific naming, and have DIERef have a "m_file_index" and "m_file_index_valid" instead of the dwo specific members. As long as both OSO and DWO files can be found from the user_id_t API calls we are all good. Not sure if this addresses all of your issues or not.

If all of your concerns are not clarified above, can you clarify what is still being overlooked? Both Alexander and I are usually thinking we are addressing everything you want, but we obviously still aren't, so restating your remaining concerns would help us get this patch moving.

Everything is clarified is and fine. I agree with your plan Thanks. I was trying to return the favor and clarify my own (potentially rude) responses.

[ayermolo]

In D138618#4102275, @labath wrote:

In D138618#4094933, @clayborg wrote:

• My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.

I do want to state that if we fix things the way I describe it will work seamlessly with OSO or DWO files. The current state of things is the DWO stuff only uses the fancy DIERef constructor and fills in the dwo number correctly only to have it overwritten in SymbolFile::GetUID(...). The SymbolFile::GetUID(...) is needed for OSOs currently because the DIERef that SymbolFileDWARF (which is used for OSO) doesn't correctly create DIERef objects since they always return llvm::None for SymbolFileDWARF::GetDwoNum(). But the new API will have SymbolFileDWARF::GetFileIndex() to be used instead of SymbolFileDWARF::GetDwoNum(), and the file index will be set correctly for both DWO and OSO files. We can then change DIERef away from DWO specific naming, and have DIERef have a "m_file_index" and "m_file_index_valid" instead of the dwo specific members. As long as both OSO and DWO files can be found from the user_id_t API calls we are all good. Not sure if this addresses all of your issues or not.

If all of your concerns are not clarified above, can you clarify what is still being overlooked? Both Alexander and I are usually thinking we are addressing everything you want, but we obviously still aren't, so restating your remaining concerns would help us get this patch moving.

Everything is clarified is and fine. I agree with your plan Thanks. I was trying to return the favor and clarify my own (potentially rude) responses.

Great, thank you for your and Greg feedback. Let me modify my changes according to Gregs suggestion.

[ayermolo]

Implemented Gregs suggestion. Also consolidated three diffs back into one.
To make it clear the scope of changes, and for ease of testing.

[clayborg]

Very clean patch now, just a few nits about asserts!

lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp

129

Does this assert really need to exist? Why would we not require a .dwo file (old code) be able to index? Can we remove this assert? It seems wrong?

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

403–404

Does this assert really need to exist? Why would we not require a .dwo file (old code) be able to index? Can we remove this assert? It seems wrong?

lldb/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp

52–53

Does this assert really need to exist? Why would we not require a .dwo file (old code) be able to index? Can we remove this assert? It seems wrong?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1404

Pavel: note we now really on "SymbolFileDWARF::GetDie(user_id_t)" to be the one source of truth when finding a DIE. We could make "SymbolFileDWARF:GetDie(DIERef ref)" be the one source of truth and then have "SymbolFileDWARF::GetDie(user_id_t)" just create a local DIERef and then call "SymbolFileDWARF:GetDie(DIERef ref)" if that would be cleaner.

1682

Pavel: note we now really on "SymbolFileDWARF::GetDie(user_id_t)" to be the one source of truth when finding a DIE. We could make "SymbolFileDWARF:GetDie(DIERef ref)" be the one source of truth and then have "SymbolFileDWARF::GetDie(user_id_t)" just create a local DIERef and then call "SymbolFileDWARF:GetDie(DIERef ref)" if that would be cleaner.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

570–572

[labath]

lldb/source/Plugins/SymbolFile/DWARF/DIERef.h

21

I don't understand this sentence.

31

they're not actually protected

lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp

75

Is this the only call site of the DIERef(SymbolFileDWARF&) constructor?

If so, and if we make it such that DWARFBaseDIE::GetDIERef returns the fully filled in DIERef, then this function can just call get_id() on the result, and we can delete that constructor.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1682

+1

[clayborg]

lldb/source/Plugins/SymbolFile/DWARF/DIERef.h

19–24

31

There were for a bit to control access to this, but in reality we ended up friending too many classes and then ran into issues with the DIERefTest stuff, so we decided to make them public again. We can remove this comment.

lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp

75

This line doesn't make sense. If we got a valid DIERef back from GetDIERef(), then we just return that as it would have used the SymbolFileDWARF to fill everything in already. So we might not need that extra constructor if this is the only place as Pavel suggested.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1682

Ok. So lets do this - change "DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t uid)" to just be:

DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t uid) {
  return GetDIE(DIERef(uid));
}

And then change the current "DWARFDIE SymbolFileDWARF::GetDIE(lldb::user_id_t uid)" to be the one that does all of the work:

DWARFDIE SymbolFileDWARF::GetDIE(DIERef die_ref) {
  std::optional<uint32_t> file_index = die_ref.file_index();
  if (file_index) {
    if (SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile())
      symbol_file = debug_map->GetSymbolFileByOSOIndex(*file_index); // OSO case
    else if (*file_index == DIERef::k_file_index_mask)
      symbol_file = m_dwp_symfile.get(); // DWP case
    else
      symbol_file = this->DebugInfo()
                        .GetUnitAtIndex(*die_ref.file_index())
                        ->GetDwoSymbolFile(); // DWO case
  } else if (die_ref.die_offset() == DW_INVALID_OFFSET) {
    symbol_file = nullptr;
  } else {
    symbol_file = this;
  }

  if (symbol_file)
    return symbol_file->GetDIE(die_ref);

  return DWARFDIE();
}

[ayermolo]

lldb/source/Plugins/SymbolFile/DWARF/DIERef.h

31

Oops, forgot to remove. For one of the internal revisions I experimented with making them protected.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

198

m_offset is is bit field now, so without it clang produces error.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1682

ah, yes, great suggestion.

[ayermolo]

addressed comments

[clayborg]

Looks good to me. Pavel?

[labath]

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

403–404

That was because it a split dwarf setup, there are two compile units, two symbol files and two CU DIEs. In a DIERef, the unit offset refers to the offset of the main unit within the main symbol file (because that's globally unique), but the die offset refers to the offset in the separate file (because that's where the dies are). The indexing process needs to start with the main unit (not the one from the split file) in order for the DIERefs to come out right, and these assertions were enforcing that. Therefore, I think we should put all of these back in.

Or at least, that was the case at some point in the past... I don't know whether this has changed since then, but I wouldn't expect it to.

[labath]

Thank you for your patience. I'm really happy with the overall result here.

[ayermolo]

Added asserts back in

[ayermolo]

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

403–404

Thank you for elaborating, Will put them back in.

[ayermolo]

Had to revert, broke some build bots. :(

[ayermolo]

fixed cross-project tests, and also normal test that I somehow missed.
Need to get access to windows machine to figure out why that fails.

[ayermolo]

small clenaup

[ayermolo]

OK, managed to setup windows VM, and couldn't repro.
After reverting my fix for cross-projects tests locally, I saw test failures.
Will try to push again and see if it triggers bot build failures.

[mib]

Hi @ayermolo!

This patch is causing some failure on the macOS lldb bot: https://green.lab.llvm.org/green/job/lldb-cmake/51257/

Could you take a look ? If you don't have the time, we can revert your patch until you manage to reproduce these failures.

Let me know if you need help with that :)

[ayermolo]

In D138618#4133717, @mib wrote:

Hi @ayermolo!

This patch is causing some failure on the macOS lldb bot: https://green.lab.llvm.org/green/job/lldb-cmake/51257/

Could you take a look ? If you don't have the time, we can revert your patch until you manage to reproduce these failures.

Let me know if you need help with that :)

Sorry about that. I ran on mac and tests passed. Let me see if I can repro tomorrow.
In meantime reverted.

[ayermolo]

This comment was removed by ayermolo.

[ayermolo]

Fixed logic for DwarfMap, also removed an assert in AppleDWARFIndex::GetGlobalVariables.
Before when it would invoke GetDwoNum it will go to a virtual API that alays returned nullopt.
The ID was handled through GetOSOIndexFromUserID. Now that it's all consolidated under
GetFileIndex it's no longer an apporpriate assert I think.

[ayermolo]

removed two more asserts. It made one of the tests fail as "Unresolved".

[ayermolo]

https://green.lab.llvm.org/green/job/lldb-cmake/51484/ passed.
There were other built bots failures, but looks like it was built failure related to other diff that was part of testing. Since I only changed things on mac side, and previously other built bots passed, fingers crossed this is it. :)

[mib]

In D138618#4145644, @ayermolo wrote:

https://green.lab.llvm.org/green/job/lldb-cmake/51484/ passed.
There were other built bots failures, but looks like it was built failure related to other diff that was part of testing. Since I only changed things on mac side, and previously other built bots passed, fingers crossed this is it. :)

Thank you @ayermolo ! I really appreciate that you took the time to fix this :)

[ayermolo]

In D138618#4145804, @mib wrote:

In D138618#4145644, @ayermolo wrote:

https://green.lab.llvm.org/green/job/lldb-cmake/51484/ passed.
There were other built bots failures, but looks like it was built failure related to other diff that was part of testing. Since I only changed things on mac side, and previously other built bots passed, fingers crossed this is it. :)

Thank you @ayermolo ! I really appreciate that you took the time to fix this :)

No, problem. Thank you for bringing it to my attention. Hopefully no more reverts will be necessary. :)