In D48397#1138455, @friss wrote:

If you have a local test case with an archive that triggers the malloc behavior, can you run an Asanified dsymutil on it with your patch applied? I'd like to make sure that nothing in dsymutil is going to access one past the end of the file.

Thanks Wolfgang, I'm always excited to see improved error handling! LGTM modulo inline comment.

LGTM, Thanks!

Thanks Jason, good catch!

In D48241#1134985, @labath wrote:

This is not true. (Unlike the .apple_*** tables, ) .debug_names allows you to specify a different schema for every entry in the acelerator table. The schema is specifing using abbreviations in much the same way as .debug_abbrev specifies the schema for .debug_info. So you could easily have one abbreviation for regular classes, and a different one for objc interfaces. Currently, our .debug_names generator does not support these kinds of heterogeneous tables, but that's simply because I had no use for it. It could be added if necessary (though it will require some generalization/refactoring). OTOH, our consumer should already be able to handle these kinds of tables as input.

• Feedback Adrian

• Use type cache &
• Simplify method cache struct
• Add custom test that verifies category methods are emitted

In D48241#1134240, @dblaikie wrote:

Not sure I should get too involved in this discussion, knowing so little about Objective C - but if it seems sensible, could you provide some examples (just in comments/description in this code review) of what the DWARF used to look like and what it looks like after this change?

CR feedback

LGTM!

LGTM with Philip's comment addressed.

Can you try inserting something like this instead of the ApplyDecoratorsToFunction line and see if your problems go away?

@wraps(InlineTest._test)
def test_func(*args, **kwargs):
    return InlineTest._test(*args, **kwargs)
 
test_func = ApplyDecoratorsToFunction(test_func, decorators)

This should make sure each test class gets a fresh function object with an independent set of categories.

Look fine to me

If I correctly understand this change, this might make it possible to apply the add_test_categories decorator to an inline test. We had issues with this when introducing the swiftpr category because the methods were part of the inline test class. From the description I get the idea that now they become part of the individual instantiations? I really hope this is the case :-)

In D47539#1117114, @jasonmolenda wrote:

LGTM. If we added more knowledge specifically about kext bundle layouts, we could restrict which files we test to see if they are valid binaries - but we'd need to parse the Info.plist at the top (to get the CFBundleExecutable name, and look for variations on that prefix) and we'd need to handle shallow/deep kext bundles. Given how few files are in kext bundles besides the kexts themselves (a couple of plists), this is code is much simpler than encoding all that extra specifics about kexts.

LGTM, thanks!

Alright, let's abandon this and replace the existing uses with llvm iterators.

Don't use EnumerateDirectory

In D47535#1116430, @labath wrote:

In D47535#1116392, @JDevlieghere wrote:

In D47535#1116364, @labath wrote:

Actually, I wonder if we shouldn't just deprecate this function altogether. What was your motivation for this patch? It seems we already have llvm::fs::(recursive_)directory_iterator for this purpose. It would be great if we could use that for all new code. Have you looked at that?

My motivation is D47539. I could use the iterators directly but since the FileSpec one is based on them anyway (and adds some functionality that is actually useful) I figured I might as well use them for consistency. I'm not opposed to using the iterators directly, but won't that result in more code?

Looking back at the last refactor of this function (https://reviews.llvm.org/D30807) I think the intention even then was to deprecate/remove it altogether.

Also, I don't think that this would increase the amount of code. Looking at your patch, it seems that it could be equivalently implemented using llvm iterators as:

std::error_code EC;
for(llvm::sys::fs::recursive_directory_iterator Iter(dir.GetStringRef(), EC), End; Iter != End && !EC ; Iter.incement(EC)) {
  auto Status = Iter->status();
  if (!Status)
    break;
  if (llvm::sys::fs::is_regular_file(*Status) && llvm::sys::fs::can_execute(Status->path())
    executables.push_back(FileSpec(Status->path()));
}

which is (a tiny bit) shorter. I also find code with no callbacks more readable.

In D47535#1116364, @labath wrote:

Actually, I wonder if we shouldn't just deprecate this function altogether. What was your motivation for this patch? It seems we already have llvm::fs::(recursive_)directory_iterator for this purpose. It would be great if we could use that for all new code. Have you looked at that?

In D47535#1116274, @labath wrote:

Could we just get rid of the baton version?

• Replace custom logic with LLVM's path logic.
• Add tests.

• Address Greg's feedback

I don't know much about the VFS so I only did a superficial review.

Thanks James!

Thanks Pavel, LGTM.

Is this still relevant?

Thanks David, LGTM

• Address Adrian's comments

In D46005#1109693, @zturner wrote:

FWIW, I think the single biggest improvement one could make to the LLDB test suite runtime is to compile all inferiors up front as part of the CMake step. If you run the test suite twice every inferior is unnecessarily compiled twice.

I'm going to hold off on this until we have decided what to do for D46005. If we run the test cases as separate lit invocations, then the test format is in a better position to report this information.

I've given this some more though as I see valid points and concerns on both sides of the argument. I'm leaning towards running test cases in parallel because it offloads more work to lit at negligible cost (running make and launching the inferior are the biggest time consumers).

Increment number of errors so we have a non-zero exit code when this is hit.

LGTM