ping

change brace style

Landed with dcf5fc3f60846b5d2e436befb934493f5132ffd9

@kpdev42 it looks like @cmatthews left some comments for you that haven't been addressed yet. Would be good to address them so we can move this forward a bit.

So by depending on objdump it does mean you can only really support one target at once (unless you have an objdump built with support for al bfd targets).
Looks like that's not a new change in this Diff. but something we might want to change in the future.

The code itself looks fine to me,

Applying this locally, the regression triangles seem to be gone, was this intentionally?

In D113470#3118004, @thopre wrote:

In D113470#3117975, @tnfchris wrote:

Yes this works fine and doesn't interfere with our approach.

We took a different route here in that we COPY the lnt.cfg from outside the container into it to /etc/lnt-new.cfg and then restore it after the create command
but before the exec.

We do this since there are other settings we want to preserve, such as mail server settings etc.

Sounds like create-config should be modified to honor an existing config file so that you could just use a ADD in your Dockerfile

Yes this works fine and doesn't interfere with our approach.

Can we not run tox in the CI?

Thanks!

LGTM, thanks!

In D96005#3057023, @tstellar wrote:

Hi, does anyone have time to review the latest version of this patch?

In D110141#3012518, @thopre wrote:

In D110141#3012486, @kpdev42 wrote:

I think it is no need to revert 6ba06570364b936c6bfb631e9d2e4f1374446bad - it looks like right solution
I've moved my changes to https://reviews.llvm.org/D110174

Argh sorry too late. Should I revert everything then or are we all good?

This was already landed as part of D110141 :)

This is weird, your base commit is on https://reviews.llvm.org/rLNT6a3f7bbaa221aa13acb72947363acbc95a1bf583, but 6ba06570364b936c6bfb631e9d2e4f1374446bad was identical to this original change.

Hmm 6ba06570364b936c6bfb631e9d2e4f1374446bad is a very odd commit... why did that go in without any reviews..

Thanks! looks good to me but @thopre do you know what's up with the CI?

Yes, thanks both!

In D109226#2982046, @thopre wrote:

In D109226#2982029, @tnfchris wrote:

Hmm the last time I upgraded pyyaml it started complaining about the interface we're using being insecure and a security issue. is that no longer the case?

I remember it required slight tweaking.

I didn't get any warning, except for pip complaining about being run as root but that's a separate issue

Hmm the last time I upgraded pyyaml it started complaining about the interface we're using being insecure and a security issue. is that no longer the case?

In D69056#2981915, @thopre wrote:

In D69056#2981910, @tnfchris wrote:

Thanks! this looks complete now.

I guess the only question is, should we bump up the version number? to make a clear line in the sand? otherwise it's hard to tell people which version supports python 2 (in case they need to run an older one for whatever reason)? or more importantly, tell which one is python 3.

Good point. We are currently in 0.4.2dev0 so perhaps we should tag a 0.4.2 first and then pass to 0.4.3dev0?

Thanks! this looks complete now.

In D69056#2981892, @thopre wrote:

In D69056#2981873, @tnfchris wrote:

In D69056#2981864, @thopre wrote:

In D69056#2981853, @tnfchris wrote:

Ohh it's happening!

This looks good but you've missed the cPerf stuff, https://github.com/llvm/llvm-lnt/blob/f48d431f44610e339d00a33d57564c6029c4ff43/lnt/testing/profile/cPerf.cpp#L827 and lnt/testing/profile/__init__.py

What's Python2 specific in lnt/testing/profile/__init__.py?

The futures import at the top https://github.com/llvm/llvm-lnt/blob/f48d431f44610e339d00a33d57564c6029c4ff43/lnt/testing/profile/__init__.py#L4

It's already there: https://reviews.llvm.org/D69056#change-H7ZXp1LFQQlR

In D69056#2981864, @thopre wrote:

In D69056#2981853, @tnfchris wrote:

Ohh it's happening!

This looks good but you've missed the cPerf stuff, https://github.com/llvm/llvm-lnt/blob/f48d431f44610e339d00a33d57564c6029c4ff43/lnt/testing/profile/cPerf.cpp#L827 and lnt/testing/profile/__init__.py

What's Python2 specific in lnt/testing/profile/__init__.py?

Ohh it's happening!

The test HWAddressSanitizer-aarch64 :: TestCases/stack-uar-dynamic.c fails already at commit a647100b4320923b0e9d156cc3872b3be470ad98 which is before mine.
As such can't be caused by mine, I'll re-commit the patch.

Buildbot has shown a test failure in hwasan with reverting this. Investigating.

Ah I forgot the context.. Thanks @eugenis !

Ping. Sorry for the early ping, but would be good to get bootstrap working again.

reverted to diff that removed

@MaskRay was that a you prefer it that way or?

In D102059#2749302, @MaskRay wrote:

The Linux kernel has removed the interface to cyclades from the latest kernel headers[1] due to them being orphaned for the past 13 years.

libsanitizer uses this header when compiling against glibc, but glibcs itself doesn't seem to have any references to cyclades.

Sounds like cyclades interceptors should just be deleted?

Update diff

In D102059#2744070, @jakubjelinek wrote:

Alternative to this would be

#ifdef __has_include
# if __has_include(<linux/cyclades.h>)
#  include <linux/cyclades.h>
#  define SANITIZER_LINUX_CYCLADES 1
# endif
#endif

For GCC this is guard-page - 1k. The reasoning is that with any outgoing argument larger than 1k we would probe such that we maintain the invariant, but probing that 1k means we have a whole guard-size -1k left that we can use without probing. These sizes were chose as they cover about 99% of all programs (for a subset of all :)).

I don't know enough about LLVM to comment on the actual code so I will only comment on the output I see generated from the testcases.

Aha thanks for the explanation

This commit adds the parameter and update call sites
accordingly, fixing one of Flake8's F821 warning (undefined name).

was it being indirectly imported before?

good job!

In D94791#2501557, @thopre wrote:

In D94791#2501415, @tnfchris wrote:

This seems rather random compared with D94789... so the spacing rule isn't consistent for assignments?

I'm not too familiar with the rules but I fail to see the inconsistency since this change does not modify any space around assignment, only spaces before parenthesis.

This seems rather random compared with D94789... so the spacing rule isn't consistent for assignments?

LGTM

Nice cleanup!

I agree, this makes more sense so we don't break cPerf again.

use package relative import syntax

Updated comment

was landed in 60343998f7907b77e24f01c94c48ea6c753eca7e

Add more context, to diff.