This looks good with just one nit above.

This is good too. Thanks!!!

Are

this looks good

This look good

This is LGTM as you suggested pending child reviews.

This looks fine just couple of nits inline.

This is LGTM. Thanks for your patience.

This apparently requires a rebase now.

Why do we have svg and za in different register sets?

This looks good overall thanks for doing the patch split makes the review way less overwhelming.

In D157846#4644241, @DavidSpickett wrote:

In this test can we figure out whether SVE was read from Streaming mode or normal SVE mode?? and if yes may be add a check for that.

Not sure what you mean.

If you mean:

• write non-streaming SVE with one set of values
• write streaming SVE with another
• assert that lldb read the correct one

There are a couple of issues there:

• If lldb read the inactive mode, it would fail with unavailable register due to a failure down in lldb-sever. These tests would already catch that.
• The kernel and the architecture say that the values of the registers for the mode you are coming from are wiped out on mode switch. So we wouldn't be able to truly read those values even if we were able to make the ptrace call and get some data.

So it's covered, but does not/ can not be covered by checking values. It'll be correct values, or a complete failure to read.

In this test can we figure out whether SVE was read from Streaming mode or normal SVE mode?? and if yes may be add a check for that.

This patch requires rebasing does not apply cleanly on top of its parent.

Can you add more description to this patch. at the moment reading it without SME context in mind does not provide enough information to figure out what we are doing.

The patch looks to implement all stuff needed at once lets break it down into three patches instead:

1. First patch that just adds SME register set and does not implement RegisterContext Read/Write routines.
2. Implement register context read/write for SME registers
3. Implement size re-configuration after changing size of the SME vector.

Thanks!

In D156687#4628743, @DavidSpickett wrote:

First thing to note is that WriteRegister also behaves this way, but there it is more appropriate because it updates only part of the buffer before writing it out in its entirety. Useful to know where the pattern came from though.

You would need roughly the following per WriteXYZ:

-      error = WriteTLS();
+      error = WriteTLS(src);

-Status NativeRegisterContextLinux_arm64::WriteTLS() {
+Status NativeRegisterContextLinux_arm64::WriteTLS(const void* src/*=nullptr*/) {

-  ioVec.iov_base = GetTLSBuffer();
+  ioVec.iov_base = src ? const_cast<void*>(src) : GetTLSBuffer();

-  Status WriteTLS();
+  Status WriteTLS(const void* src=nullptr);

We can assume that the buffer is the same as the data to be written back if it's something static like TLS. For SVE/SME, we would have resized our buffer first, so it holds there too.

The added complexity isn't that much but I think it adds more thinking time for a developer than it potentially saves in copying time. I already feel like the separate xyz_is_valid is enough to think about and having a potential second data source just adds to that load.

From the header docs it seems I was right that this is used primarily for expression evaluation:

// These two functions are used to implement "push" and "pop" of register
// states.  They are used primarily for expression evaluation, where we need
// to push a new state (storing the old one in data_sp) and then restoring
// the original state by passing the data_sp we got from ReadAllRegisters to
// WriteAllRegisterValues.

Which you would be doing a lot of in a formatter for example, but you'd get better savings implementing a more efficient packet format to do all that at once, I guess.

QSaveRegisterState / QRestoreRegisterState packets call it as part of expression evaluation, though in theory it's not always that. That's an lldb extension anyway so we're in control of it at least. In theory this could be used to restore state that is not just the previous state but I don't know how you'd trigger that from lldb.

The other use is NativeProcessLinux::Syscall which is sufficiently rare we can ignore that.

I did do a very rough benchmark where I printed the same expression 2000 times, so each one is doing a save/restore. Once with the code in this review right now, and again with this potential optimisation added to GPR/FPR/TLS (I'm on a Mountain Jade machine without SVE). Caveat shared machine, made up benchmark, etc. but all runs of both hovered between 16 and 17 seconds. Neither seemed to be consistently lower or higher than the other. Doesn't mean this isn't a speedup in isolation but if it is, it's dwarfed by the syscalls and packets sent back and forth.

I don't know about expensive literally but I did wonder if we could rely on the NativeRegisterContextLinux_arm64 buffers not being modified while the expression runs. Then do what I think you mean which is just flush them all to the process.

I'll need to work out exactly when SaveAll/WriteAll is and can be called. Part of me wonders why we even have such methods if you could just flush the existing buffers, but it wouldn't be the first time we've duplicated things.

I have a feeling that expressions are becoming an expensive operation with amount of data we need to move back and forth between various buffers. Is there a way we can optimise this may be write register directly from source buffer.
Also how much minimum data we need to move when SME is enabled?

This looks fine but instead of adding a new test cant we add MTE control register tests to already available MTE tests?

In D157274#4565313, @DavidSpickett wrote:

In case it's useful, this is what Linaro runs currently for flang:

clang-aarch64-full-2stage       
clang-arm64-windows-msvc        
clang-arm64-windows-msvc-2stage 
clang-aarch64-sve-vla           
clang-aarch64-sve-vla-2stage    
clang-aarch64-sve-vls           
clang-aarch64-sve-vls-2stage    
flang-aarch64-dylib             
flang-aarch64-sharedlibs        
flang-aarch64-out-of-tree       
flang-aarch64-release           
flang-aarch64-debug             
flang-aarch64-latest-clang      
flang-aarch64-rel-assert        
flang-aarch64-latest-gcc

To give you an idea why I'd like to replace an existing configuration rather than adding a new one on top of all that. Feel free to disagree of course, or suggest another home for reverse iteration.

Never mind got the answer from the summary.

Thanks David for adding this builder. Re debug builder do you plan to remove the flang-aarch64-debug after the new builder is up and running. I guess if we add a new builder with DLLVM_REVERSE_ITERATION=ON we ll loose the ccache advantage given many files will get built twice for the same set of changes.

This looks good. Just a nit the summary seems to be using a lot pronouns. Could you kindly improve summary a little and add a one liner about expr_func and how it tests save/restore for future readers of this log.

Looks legit. Thanks

@farzon thanks for this fix. Please upload a full context diff using

git diff -U999999 --no-color HEAD^

I guess there is not much gain from telling the user between sve/ssve mode for the moment. Can we defer this patch till SME registers are implemented?

This looks very good much simpler from the first version. just a minor nit.

This looks good just a minor suggestion inline.

This update fixes some bugs after testing.

Sounds ok to me. We can always fix it if starts to cause trouble.

This seems to have broken https://lab.llvm.org/buildbot/#/builders/245/builds/10311

This looks good to me, just a minor nit above. I have not run the test myself.

This looks good.

Kindly fix flang unit tests for Windows before relanding. Thanks

This change has broken various unit tests on AArch64 Windows platform. I going to revert this change temporarily.

Hi Aaron,

In D148761#4343870, @aaron.ballman wrote:

In D148761#4343528, @omjavaid wrote:

In D148761#4343485, @dpaoliello wrote:

@omjavaid this change DID NOT CAUSE THE TEST FAILURE, please see the previous comments where we were discussing this:

In D148761#4326518, @aaron.ballman wrote:

In D148761#4324327, @bolshakov-a wrote:

Thanks! Honestly, I don't know how to reproduce that test results on my Win-x86-64 platform to reveal failure reason.

CC @zturner and @clayborg -- this lldb builder has been down for 11 days and there are several failures unrelated to the one in this patch, otherwise I would have reverted this instead of waiting for the author to get back from vacation. Do either of you want to fix or revert to get the bot back into a good state?

I am the owner of lldb-aarch64-windows and i have verified that exactly this commit caused the test to fail. The builder was failing a few test earlier thats why this particular failure got masked.

Did you verify that the failure was because the changes were incorrect? These changes are correcting a bug and it's expected that downstreams like lldb may have to react to that sort of thing breaking their tests. A revert ~10 days after the failure landed is not exactly a timely revert -- now all the downstreams that did react to this change will have to react again, which is why I'm curious what amount of analysis was done prior to reverting and what details you can share about the problem.

In D148761#4343485, @dpaoliello wrote:

@omjavaid this change DID NOT CAUSE THE TEST FAILURE, please see the previous comments where we were discussing this:

In D148761#4326518, @aaron.ballman wrote:

In D148761#4324327, @bolshakov-a wrote:

Thanks! Honestly, I don't know how to reproduce that test results on my Win-x86-64 platform to reveal failure reason.

CC @zturner and @clayborg -- this lldb builder has been down for 11 days and there are several failures unrelated to the one in this patch, otherwise I would have reverted this instead of waiting for the author to get back from vacation. Do either of you want to fix or revert to get the bot back into a good state?

This caused test failure on lldb-aarch64-windows buildbot
https://lab.llvm.org/buildbot/#/builders/219/builds/2520

This introduced test failures on windows lldb-aarch64-windows buildbot.

In D148752#4283899, @manojgupta wrote:

I am building on ChromeOS. We only have headers from linux kernel 4.14 available in our build system (The actual running kernel could be a higher version).
But given these functions/struct definitions (sve::<foo>) are already available and used, why are they not used consistently?

e.g. https://source.chromium.org/chromium/external/github.com/llvm/llvm-project/+/main:lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp;l=99
already uses sve::vl_valid and there are uses of sve::user_sve_header as well.

HI Manoj
Which linux distro are you using for your cross build? I am wondering which version of gcc (aarch64-linux-gnu) are you using for your cross compilation build.
I believe sve headers are not defined in older versions of gcc or its packaged sysroot causing cross compilation build to break.
All sve:: functions and defines are just a make shift arrangement to accommodate build environment which dont have SVE headers/functions defined.