There's a build warning that needs to be fixed, but otherwise LG.

In D87333#2274572, @rupprecht wrote:

In D87333#2273506, @labath wrote:

(Sorry about the delay.) Given the current requirements, I think this patch is fine (excellent even).

That said, I'm not sure whether the original motivation for this requirement (avoiding dns lookups) is still relevant. These days, we communicate with the local debug server via socketpair(2) sockets (which wasn't the case back then), which does not require any dns lookups and is immune to a misconfigured hosts file.

The initial dns lookup may still fail, I think? e.g. my initial version of this patch had:

if (llvm::Error error =
        listen_socket.Listen("localhost:0", backlog).ToError())

If the hosts file has a bad value for localhost, that will create a bad socket. That said, I don't know how to reproduce the original issue -- maybe it isn't an issue for some other reason.

Given that none of the things touched by this patch is extremely critical (GDBRemoteCommunication::ConnectLocally is used for reproducers and the rest is test code), maybe we could use this to test the water and see whether we can start using the network stack the way it's supposed to be used ?

Many non-reproducers tests fail w/o the change to GDBRemoteCommunication::ConnectLocally, so it's at least used outside of that.

I can go either way with using localhost or using hard-coded localhost IPs. Whichever way we go, I have a slight preference for keeping the tests and code consistent, e.g. if we use hard-coded IPs in code linked into lldb to allow people with a bad hosts file to use lldb, we should use hard-coded IPs in the tests to allow those same users to develop on lldb.

• Switch back to using localhost for non-socket uses

The issue is with the new pass manager.

Looks like llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test covers this, so LGTM

In D87333#2273506, @labath wrote:

(Sorry about the delay.) Given the current requirements, I think this patch is fine (excellent even).

That said, I'm not sure whether the original motivation for this requirement (avoiding dns lookups) is still relevant. These days, we communicate with the local debug server via socketpair(2) sockets (which wasn't the case back then), which does not require any dns lookups and is immune to a misconfigured hosts file.

The initial dns lookup may still fail, I think? e.g. my initial version of this patch had:

• Compare directly against the error code instead of hopping through std::make_error_code()

This is ready for review now after scrubbing out the "localhost" changes I had earlier.

• clang-format

• Don't use "localhost" to avoid dns latency. Instead, prefer either 127.0.0.1 or ::1 directly.

In D87333#2262241, @clayborg wrote:

In D87333#2262236, @rupprecht wrote:

In D87333#2262229, @clayborg wrote:

FYI: we switched away from "localhost" a long time ago due to issues with people having a "localhost" entry in their /etc/hosts folder.

I assume you mean people *not* having a "localhost" entry?

I believe the issue was with people having a modified version that replaces localhost with some other IP address.

I didn't even realize that was an option. Yes, this patch would break that. I'll have to take another approach, then.

You might check the "git log" on any files that have "127.0.0.1" in them for details.

Do you happen to have any references (bugs etc.) for those kinds of issues?

In D87333#2262229, @clayborg wrote:

FYI: we switched away from "localhost" a long time ago due to issues with people having a "localhost" entry in their /etc/hosts folder.

SG, will land momentarily with the one formatting change. Thanks!

When trying to integrate this downstream, I'm having trouble figuring out what to do with this block: https://github.com/tensorflow/mlir-hlo/blob/master/lib/Dialect/mhlo/transforms/hlo_legalize_to_lhlo.cc#L441

Thanks for the revert.

Ok. Thanks for confirming!

Usually, tests that are marked # REQUIRES: asserts are done so because they make use of some code that is only available behind NDEBUG. However, in this case, it's outright crashing on certain inputs when not in debug mode. At a first glance, that doesn't seem like a correct fix to me -- after all, the crashing bug is still there -- but I'm not familiar with this code, so I'm probably missing something?

FYI, I reverted this (and D86413 which depends on it) in 8693ddc74371dedc742c9f3d3e4eda1da72c13ea to keep the build green, as it was causing surprising crashes when running ninja check-llvm-codegen-aarch64-globalisel in release builds.

In D84327#2230474, @fhahn wrote:

I recommitted the change in 8eded24bf46c05ffd110d521f58320cdee93866e with a fix for the issue reported by @rupprecht, which was caused due to the expander also adding existing, re-used values to InsertedValues, which we should not try to remove.

Here's as far as I could get with a repro. Sorry it's weird:

FYI, I reverted this in 38884641f28e373ce291dc5ea93416756216e536 due to the assertion failure being triggered. The reduction is taking longer than usual, so I wasn't able to finish it today. I should be able to have something on Monday.

This is the largest fix I've seen for unused variable warnings, but nonetheless looks correct.

I'm not familiar with sparc target arches, but this looks fine to me if you've been able to verify it fixes things.

FYI, reverted in 3c39db0c4452218c967a8ac3ad48144fbf1159ff. See http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/19573/steps/build%20stage%201/logs/stdio for a breakage that's shown up in buildbots.

Thanks for the quick review!

• Use logging.error
• Remove trailing stop
• Edit message to indicate that matching tests weren't found (user error)