• Compile regex only once

From what I understand this means D91378 is no longer necessary. thanks!

• Add a comment for the orphan in the doc page.

In D94489#2503662, @tschuett wrote:

There is no syntax highlighting for Python in sphinx? The code in
https://lldb.llvm.org/python_api/lldb.SBDebugger.html#lldb.SBDebugger
is hard to read.

Even the line breaks between the source code and web page differ.

I merged the Python API now into the normal website (see https://teemperor.de/pub/lldb_website/python_api.html# for how this looks). So cross-referencing and the website search now work for everything.

• Merged Python API and normal website.

• Use the old output directory name.

I generated the current website here: https://teemperor.de/pub/sbapi/

LGTM modulo the warning messages still referencing 2 instead of 3.

LGTM, thanks!

Some comments about that this still prints 255, but otherwise this is looking good.

I just realized that I got the char sign also mixed up. It's an unsigned char in LLDB, so the 255 is wrong (But I believe our char signedness handling is not correct in any C language). But the fact that we don't print YES/NO for that bitfield is right (just the int value is wrong).

I think the proper fix here is to warn about BOOL bitfields with size 1 if BOOL is a typedef for signed char. I can make a Clang warning for that. I guess on the LLDB side we should make it clear that BOOL here has an invalid value (and then print the int value as we currently do)?

In D93421#2459853, @labath wrote:

This seems suspicious to me, on a couple of levels. You claim that BOOL is an unsigned type, but the apple documentation says "BOOL is explicitly signed so @encode(BOOL) is c rather than C even if -funsigned-char is used." Which one is true? Could this have something to do with the fact that the documentation assumes obj-c (which does not have a native bool type) but in lldb, we use obj-c++ (which, I guess, inherits bool from c++) everywhere?

If BOOL is unsigned, then it's not clear to me why would Scalar::ExtractBitfield be sign-extending anything. OTOH, if it is _signed_, then sign-extension seems to be the right behavior.

I think we may have some deeper problems with the handling bitfields whose underlying types are 1 byte long. Lldb (I think, correctly) sign-extends all signed bitfields, except the signed char one:

(lldb) p a
(A) $0 = {
  bool_ = true
  signed_char = '\x01'
  signed_short = -1
  signed_int = -1
  unsigned_char = '\x01'
  unsigned_short = 1
  unsigned_int = 1
}
(lldb) p/x a
(A) $1 = {
  bool_ = 0x01
  signed_char = 0x01
  signed_short = 0x0001
  signed_int = 0x00000001
  unsigned_char = 0x01
  unsigned_short = 0x0001
  unsigned_int = 0x00000001
}

This patch does not have any impact on this behavior, but the fact that signed char comes out as '\x01' seems suspicious. This is what gdb does for the same input:

(gdb) p a
$1 = {bool_ = true, signed_char = -1 '\377', signed_short = -1, signed_int = -1, unsigned_char = 1 '\001', unsigned_short = 1, unsigned_int = 1}
(gdb) p/x a
$2 = {bool_ = 0x1, signed_char = 0xff, signed_short = 0xffff, signed_int = 0xffffffff, unsigned_char = 0x1, unsigned_short = 0x1, unsigned_int = 0x1}

Finally, even though I'm not sure this is the right solution, I am sure that GetValueAsSigned should get the same fix as GetValueAsUnsigned

Whether BOOL is signed char or bool is apparently depending on the architecture (???). See ClangExpressionSourceCode::GetText, Clang's __OBJC_BOOL_IS_BOOL macro and then there is also some preprocessor shenanigans for non-Clang ObjC (?) in the objc.h header from what I can see. Whether C++ is active is never actually checked anywhere (the bool type is coming from stdbool.h which is unconditionally included by objc.h).

I believe rsmith is in GMT-8, so I assume this won't get a fix soon and I went ahead and reverted in 22ccdb787024e954318e35fcf904fd4fa36f5679

There area few failures once I actually create the lookup table, so I put this on my TODO list.

LGTM. I assume that's the result of investigating Vedant's zombie generation script?

In D92848#2442613, @martong wrote:

In D92848#2442609, @martong wrote:

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

Yes, that'd would be super.
Other than that, if you could make sure that the ASTImporterLookupTable is initialized then noload_lookup would not be needed and we could simplify so much in clang::ASTImporter.

Hmm, as far as I remember, in the past I tried this. And bumped into some issues, because during the traverse we had to load Decls with external storage and that introduced a new and uncontrolled import...

• Clarify ClangASTContext destructor code.

• Expanded test with mixed debug info/module types (thanks Shafik!)

• Now using one setting with a backing enum (Thanks Jonas)
• Gave up on imposing my doxygen code style on the project (Thanks Adrian).
• Expanded test (Thanks Shafik)

In D92784#2437994, @JDevlieghere wrote:

Would it make sense to convert import-std-module to an enum with values True, False and Fallback instead of introducing a new option and then changing the default over time?

I'll go ahead and land this to unbreak the bots using Clang modules. Just using this for post-commit review.

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging? Given that we most likely never want this to happen within LLDB, this seems like a straightforward change.

There are several LLDB tests failing with this change. The TestImportBaseClassWhenClassHasDerivedMember.py is probably the only relevant one. The others are probably failing for the same reason but with a more convoluted setup. I looked into the failure and it seems this patch is now skipping the workaround in ImportContext that was introduced in D78000 (as the Importer.GetAlreadyImportedOrNull will give us a DeclContext and then we just use that as-is). If you remove the if (!DC || !LexicalDC) before the if ((Err = ImportDeclContext(D, DC, LexicalDC))) this should keep the old behaviour.

LGTM, thanks! Bonus points for the emotional rollercoaster in the review history.

I had to add a lldb-noinit alias that creates a completely uninitialized lldb (as the default init file for the tests sets a cache path).

In D92209#2437416, @martong wrote:

Ping.
@teemperor please take a look if you have some time. This is a really important patch which may influence some future directions regarding the ASTImporter.

I was a bit too quick with updating that one, doesn't seem to pass the tests. Two small fixes: