Hi Mandeep,

Hi Mandeep,

This intrinsic got added to gcc a while ago and should become available in the upcoming gcc 9 release.
In gcc however, the prototype of the intrinsic is slightly different (see https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html):
type __builtin_speculation_safe_value (type val, type failval)
It provides a second optional argument "failval". From the gcc documentation: "The function may use target-dependent speculation tracking state to cause failval to be returned when it is known that speculative execution has incorrectly predicted a conditional branch operation."
So, when implementing the intrinsic using a speculation barrier such as lfence, that failval argument doesn't have any effect. However, when lowering the intrinsic using speculation tracking similar to how that's used in SLH, this failval parameter is used to return a non-zero value on miss-speculation, in case the developer prefers that over the default zero value.

Thanks for picking this up, Zola!

Hi Tim,

In D57806#1415955, @abrown wrote:

Thanks for the review. I don't have committer rights; could you merge this for me or point me to how to get access?

LGTM, thanks for fixing this!

This seems quite obvious.
Typically when we change the behaviour of code, we aim to add a unit/regression test to check that indeed the code now has the intended behaviour (and mostly to make sure future changes do not regress on that expected behaviour).
That being said, this is a change to debug output, for which we typically do not add regression tests.

Updated approach to use the register scavenger to find an available temporary register. In the rare case when no temporary register is available, fall back to inserting a full speculation barrier.

In D55929#1358920, @zbrid wrote:

Hi Kristof,

Thanks for writing this code! I had an easy time understanding what you were doing.

I was discussing this SLH implementation with Chandler and he suggested that it may be useful to write a design doc similar to the one for the x86 implementation, so other people in the community can understand and discuss the current/future design with the ARM specific details laid out. What do you think?

In D54896#1354951, @zbrid wrote:

Hi Kristof,
Sorry about this late review. I wanted to make sure that I understood your implementation of SLH for ARM, so I took a look and made a few comments with some questions on this first patch. Let me know your thoughts. I'm planning to take a look at the other commit you made a few days ago related to SLH too. Hopefully I didn't make too many comments that were already addressed in the follow up commit.

Any feedback on the idea of an alternative, much lighter Conduit API client (since we only use a subset of Phab features) where it's also easier to (by default) hook linters or various other tools to? Not sure how others feel about Arcanist especially considering it covers a massive set of APIs beyond what's commonly used here and a local PHP installation just to run it is often undesirable (A simpler one-file client in Python should suffice, especially considering that many are changing their workflows due to the Git migration).

In D52803#1349005, @aditya_nandakumar wrote:

By default the CSE is enabled during IRTranslator and Legalizer now (disable with -enable-cse-in-legalizer/irtranslator=0).

I naively wonder if unconditionally enabling a CSE optimization even at -O0 is going to lead to a poorer debug experience in some cases?

Fix 2 issues flagged by -verify-machineinstrs when testing this patch on more code.

This updated diff addresses Oliver's final comment, namely about what to do when the program uses X16, e.g because of use of inline assembly specifically requesting to use X16.
Besides all the options I came up with previously, there is a 4th option: prevent speculation by inserting DSB SYS/ISB instruction pairs. Conceptually this is not unlike using lfence in X86SpeculativeLoadHardening.
I believe the pros/cons of this approach are (compared to the 3 options I listed previously):
Pros:

• A relatively simple implementation.
• Still keeping the advantages of doing speculation hardening very late.
• No silent non-protected code.
• This alternative protection mechanism is expected to only be needed for a very small amount of code.

Cons:

• This likely has higher overhead than if we could still use a data flow mechanism to track when control flow miss-speculation happens.

There currently isn't even a user interface to reserve X16.

X16 can be reserved by the user using inline assembly, either with a clobber or named register variable. I can think of a few cases where this might happen in real code:

D51432 uses it in libunwind to implement unwinding through functions using return-address signing.
Inline assembly which contains a function call will need to clobber X16 and X17.
If moving this pass to before register allocation would add a lot of extra complexity, maybe this could also be solved by copying the taint into SP before each inline asm block, and back out afterwards, like we currently do for calls?

• Addressing most of Oliver's comments.

LGTM now. I'm not an expert on the clang attribute mechanics, so am relying on Aaron's judgement/review for that part.
Thanks Zola!

In D54555#1301084, @aaron.ballman wrote:

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

This patch looks fine to me, assuming the TSV110 core implements Armv8.2 including the optional extensions ARMv8.2-FP16, ARMv8.2-DotProd, ARMv8.2-FHM and SPE.

Add a sentence on statuses involved when triaging.

I guess you're mainly asking review on this to get opinions on the approach to introduce new pseudo instructions just to avoid cycles during DAG combining and/or lowering?
From the patch, I furthermore guess that introducing a new pseudo leads to quite a bit of code duplication/violations of the "don't-repeat-yourself" principle in the instruction matching patterns?

In D53121#1261408, @srhines wrote:

This LGTM, but we should wait to hear from Kristof before submitting.

Hi Aaron,

In D51862#1229129, @danilaml wrote:

I'm also using pycodestyle 2.4.0. The error/warning was not about raw strings in regexps, rather illegal escape sequences, i.e.:

.\lnt\server\db\regression.py:58:20: W605 invalid escape sequence '\d'

I think using raw strings is better than using something like \\d.

Btw, I doubt that I can commit to llvm repo, so I need somebody to commit this for me. Thanks!

I am as excited by this as the other reviewers.
Thank you very much for this, Matthias!

I'm not an expert on many of the areas touched by this patch, but it looks fine from me from a high-level point-of-view, modulo a few nits I have on a few comments.

update diff with full context

In D48580#1145509, @efriedma wrote:

I believe this is aiming to implement the -ffixed-reg command line option from gcc.
The documentation is at https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html:

It's clear from your description that the run_under _mutateCommandLine isn't a "private" function in the module as is, and therefore removing the under score makes sense to me.
Maybe it'd be better for the mutatePlan in perf.py to be implemented in a different way so that _mutateCommandLine really can be private in the run_under module.
I'm afraid I don't have enough experience here to know whether that would be a reasonable goal, implementable with reasonable effort.
I hope @MatzeB could share his opinion on this.

LGTM.

In D47581#1119035, @john.brawn wrote:

Maybe the best way to test this is to add a helper function in tests/server/ui/V4Pages.py to extract the full content from a specific field from the table (i.e. including all HTML tags) and check against exact HTML content?

What do you think - would that be feasible and useful to implement?

After trying some things I think it will be easier to extract just the destination of the links and check that those are correct, especially as it's just the destination of the links that was wrong.

Hi John,

Oh - and I indeed also ran the source code through YAPF - thanks for that pointer, I didn't know that existed.

Many thanks for the detailed review, Jonas!
I think I've addressed all your comments - very useful feedback!

It does seem like a wiki would be nice to maintain this kind of information. In the absence of that, I think that a file in the test-suite repository, or a page in www are about equally easy/hard to maintain: it requires commit access to make any changes.
A file in www in theory could be more visible as it becomes part of the llvm.org web pages. That being said, source code is also viewable online, so it's easy to browse this text too.

In D45000#1086160, @danilaml wrote:

@MatzeB Is there something else I should do for this to be commited? First time using the Phabricator so I'm a bit unfamiliar with the process.

Updating diff to make it add the script to llvm/utils/Reviewing.
I chose to put it in a new folder "Reviewing", since ideas for further scripts related to reviewing have been made on this thread.

I hope that in the long run, we can get rid of this cPerf.cpp C++ implementation, and instead make use of "perf data convert --to-ctf" to convert perf.data files into a CTF format which should be easier to parse in python.
At the time this C++ implementation was added, the "perf data convert --to-ctf" functionality tended not to be compiled in to perf as distributed by the main linux distros.
Looking at a reason stack overflow issue about this suggests that the situation may not have changed much (https://stackoverflow.com/questions/43576997/building-perf-with-babeltrace-for-perf-to-ctf-conversion), hence we'll probably need this C++ implementation for some time to come.

In D46192#1082638, @JDevlieghere wrote:

In D46192#1082049, @lebedev.ri wrote:

This should probably go into llvm/utils, not the root.

It would also be good to have the opposite script - "given git diff / commits, whom should i add as reviewers?"

I once wrote the following one-liner to do this. It works pretty well but it can be *very* slow. It will suggest a top 10 of reviewers for your staged changes.

git diff --name-only --cached | xargs -n 1 git blame --porcelain | grep \"^author \" | sort | uniq -c | sort -nr | head -10

It could evaluate CODE_OWNERS.txt, git log / git shortlog -sne of the changed files, git blame.

In D46192#1083794, @javed.absar wrote:

Hi Kristof. Thanks for this.

One more heuristic that might help getting quick reviews:

3. If there is overlap between the files touched by the patch-under-review and and a 'reviewer' of recent changes to same file, that person may be a good reviewer.
   
   This heuristic is helpful where original implementor may have moved on, but new active reviewers in same area are happy to review. Some commit messages have 'reviewers' listed and that could be used.

In D46192#1082214, @philip.pfaffe wrote:

Do you plan on actually commiting this to the repository, or would you like any feedback nontheless?

In D46215#1083481, @phosek wrote:

In D46215#1081943, @kristof.beyls wrote:

I have some doubts this is the right long term approach.

As https://android.googlesource.com/kernel/common/+/c0385b24af15020a1e505f2c984db0d7c0d017e1%5E%21/#F4 indicates, there are others who have use cases for reserving other registers than x16,x17,x18. (The linked patch reserves x1,x2,x3,x4,x5,x6,x7).
With that indication, I think it may be better to bite the bullet now and implement full support for gcc command line option "-ffixed-reg", where reg is, according to the gcc documentation: "reg must be the name of a register. The register names accepted are machine-specific and are defined in the REGISTER_NAMES macro in the machine description macro file.".
For the full documentation of this option, see https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html.

What do you think?

Thanks,

Kristof

That was original proposal bug @chandlerc suggested in our offline discussion that it's not easily doable. It seems like we might be able to duplicate the same code for all remaining registers which will require a lot of duplicated logic but it should work? However, I might be missing some details of the register allocator which will break.