I'd recommend committing the NFC changes before this, no need for a separate review.

In D95071#2511105, @arsenm wrote:

In D95071#2511070, @scott.linder wrote:

Thank you for the patch, I definitely think this is a step in the right direction! However, I think we are still broken in common cases, the first of which can be seen by simply removing amdgpu_kernel from llvm/test/CodeGen/AMDGPU/stack-pointer-offset-relative-frameindex.ll and noticing that the soffset of the buffer_load_dwords in %bb.1 do not correctly refer to the frame pointer, they are constant 0 just like in the kernel case:

The frame index being separated from the memory operation is fine. 0 is always an acceptable soffset, kernel or not. The vaddr will be interpreted as the absolute address. If the frame index is materialized by a move, it will be expanded such that the address placed in the mubuf vaddr should work

Thank you for the patch, I definitely think this is a step in the right direction! However, I think we are still broken in common cases, the first of which can be seen by simply removing amdgpu_kernel from llvm/test/CodeGen/AMDGPU/stack-pointer-offset-relative-frameindex.ll and noticing that the soffset of the buffer_load_dwords in %bb.1 do not correctly refer to the frame pointer, they are constant 0 just like in the kernel case:

I'm still working through the whole patch series (thank you for the careful breakdown of the changes and their dependencies, it has been a big help!) but I'm going to be out for a bit around the holidays and wanted to post at least some of the feedback I do have.

No other target overrides GetDefaultDwarfVersion with the same implementation, and in the future when we make changes we can add them at the points in the hierarchy where they make sense semantically.

In D92483#2463773, @rampitec wrote:

In D92483#2463314, @pvellien wrote:

In D92483#2458755, @rampitec wrote:

But I still do not think this is a right thing to do to accept amdhsa on SI, even if you turn off flat instructions. SI does not support HSA.

As far as I the support of HSA in SI processors. Tony suggested that gfx60x processors should support AMDHSA OS because it is capable of supporting OpenCL 1.2 which not need generic pointers. This topic was discussed in amdgcn weekly and using MUBUF instructions for global address space in gfx60x is decided. The same is also documented in AMDGPU User Guide earlier.

But it is not. In fact AMDGPUUsage.rst says that SI does NOT support amdhsa and it is only supported starting from gfx700:

**GCN GFX6 (Southern Islands (SI))** [AMD-GCN-GFX6]_
-----------------------------------------------------------------------------------------------------------------------
``gfx600``  - ``tahiti``    ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
                                                                   support
                                                                   generic
                                                                   address
                                                                   space
``gfx601``  - ``pitcairn``  ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
            - ``verde``                                            support
                                                                   generic
                                                                   address
                                                                   space
``gfx602``  - ``hainan``    ``amdgcn``   dGPU                    - Does not      - *pal-amdpal*
            - ``oland``                                            support
                                                                   generic
                                                                   address
                                                                   space

It is only amdpal listed here.

What was discussed is that we can use buffer instructions for global on SI even if amdhsa is not supported.

LGTM, thank you!

In D93211#2454945, @tambre wrote:

In D93211#2454859, @epastor wrote:

In D93211#2452405, @tambre wrote:

CC'ing @epastor as a FYI for the assembly parser bug.

Pretty sure the issue you're hitting will be in AsmParser, not MasmParser - you're not using MASM here, you're using GNU-style assembly.

But - the bug will be the same regardless. I'm not sure who the right expert is on AsmParser at the moment?

Code ownership in LLVM generally feels hazy to me. CODE_OWNERS.txt seems to be mostly outdated.

@scott.linder You recently touched CFI assembly parsing, maybe you'd be interested in taking a look at the bug we encountered here too.

The change makes sense to me, but at this point it might be good to add a comment for find_first_existing_vc_file to say what it does and what each possible return value means. Maybe something like:

In D76519#2449652, @MaskRay wrote:

Thanks for refactoring it:) I dug up a bit of history. @asl introduced the variable shouldEmitMoves in rL123474 and espindola introduced the member function needsCFIMoves.
I cannot find "CFI move section" in standards so it is probably not a standard term. Should we use a more appropriate term to best capture the intention?

In D78778#2449409, @MaskRay wrote:

In D78778#2449379, @scott.linder wrote:

In D78778#2449342, @MaskRay wrote:

I think the thing we cannot get past is: ExceptionHandling::DwarfCFI is called exception handling and thus we expect it to be only useful with C++ (or other language) exceptions. However, the practice is that we overload it for non-exception .eh_frame/.ARM_extab/.ARM_exidx generation as well. So, adding a behavior to ExceptionHandling::None and introducing SupportsDebugUnwindInformation just seem to bring more complexity to me.

If ExceptionHandling were called something else, this might be less misleading.

Right, I am open to suggestions, but it also just mirrors the implementation in that the code for doing the debug-only, no-exceptions unwind information is still contained in EHStreamer/DwarfCFIException. Those could also be renamed, but then most of the code in them is irrelevant to/complicates the debug case.

That's how I ended up at "make a new streamer that is honest about being for debug-only DWARF CFI", and it happened to be nearly empty, so I didn't even have it share code with the exception variants. Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?

Yes, some renaming can help. I believe MCAsmInfo has sufficient states to encode all the needs.

Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?

Having both UnwindTable and ExceptionHandling enums sounds good. isCFIMoveForDebugging (the name confused me quite a bit) probably should be folded into the new representations.

In D93069#2447155, @silvas wrote:

Thanks!

You might want to link to the ProgrammersManual directly from here as well.

In D78778#2449342, @MaskRay wrote:

I think the thing we cannot get past is: ExceptionHandling::DwarfCFI is called exception handling and thus we expect it to be only useful with C++ (or other language) exceptions. However, the practice is that we overload it for non-exception .eh_frame/.ARM_extab/.ARM_exidx generation as well. So, adding a behavior to ExceptionHandling::None and introducing SupportsDebugUnwindInformation just seem to bring more complexity to me.

If ExceptionHandling were called something else, this might be less misleading.

In D78778#2449310, @MaskRay wrote:

Could you just add ExceptionsType = ExceptionHandling::DwarfCFI to AMDGPUMCAsmInfo.cpp? You can still make Asm->needsOnlyDebugCFIMoves so that .debug_frame will be generated while .eh_frame is not.

I do feel that we have a bunch of states which should be available to represent various needs (.eh_frame only, .debug_frame only, both, none) and support various options (-fasynchronous-unwind-tables), just probably that the states are not well organized...

This was our first approach, but we found other places which were sensitive to ExceptionsType being set. Maybe those aren't present anymore; is anyone aware if that option should affect CodeGen? I can take another look now, and see if I can point to the issues we had.

In D78778#2447314, @dblaikie wrote:

Sorry, I wasn't really able to follow that description. I'll try to clarify/rephrase my question which might help:

Under some conditions, eh_frame is generated even under -fno-exceptions, right? And you'd like debug_frame to be generated instead/as well, in that case? I'm trying to understand why the generation of these two things (eh_frame/debug_frame) isn't close enough together as to make the fix for this issue be a single conditional somewhere, that picks whether which name to use (& some associated properties, like whether it's ALLOC).

Could you explain the codepath that generates eh_frame under basically the conditions you would like, and why that codepath can't handle being modified to generate debug_frame instead?

Delete UnwindStreamer, merging the behavior into DwarfCFIException. Also rename
the MCAsmInfo property to SupportsDebugOnlyCFI to be more direct/accurate about
the semantics.

Small proposed addition to try to encourage the migration to the no-explicit-N form, let me know what you think.

In D78778#2444445, @dblaikie wrote:

(I'm probably not the best person to review this - just chimed in on the patch structure/testing issues, but here are some naive questions)

In D78778#2301155, @RamNalamothu wrote:

In D78778#2300827, @probinson wrote:

I haven't touched the streamer side of AsmPrinter in a long time, but...
On PS4 we default to -fno-exceptions and while the section ends up named .eh_frame instead of .debug_frame the content is fine; we've had no complaints from our debugger folks. Aside from the introduction of the .cfi directives (which you do want), I'm not aware that there's any codegen consequence to this combination.
So, I'm not clear why a special streamer is really necessary.

A special streamer is a straight forward and minimal changes approach we could arrive at as a means to solve the following concerns observed with the current implementation.

1. Today, with -fno-exceptions, the command line option --force-dwarf-frame-section of llc doesn't do what it should as per D67216 i.e. a .debug_frame is not generated

Does this change address (1)? Should there be a test for --force-dwarf-frame-section to demonstrate this fix? (what cases did D67216 address? that are distinct from this case that it misses)

Rebase over changes which eliminate the spurious newlines in the assembly
output of the DebugInfo test.

Rebase the original version of the patch again; this is the same diff that was
approved already, so I am going to just go ahead and commit it. If anyone has
any objections I can open another review to address them.

OK, thank you both for taking a look! I didn't go and update all of the failing test cases at first, because I wanted to avoid it if we decided to go the "type: is optional" route. I'll update them all and post to the review.

In D92690#2442357, @dp wrote:

I'd have reverted to using IDLoc because the error message is clear enough.
I'm sorry that my change caused such trouble.
The better is the enemy of the good :-)

Rebase over https://reviews.llvm.org/D92084

Alternatively I'm open to fix this in the caller if the (EndOfStatement, "\0") value is correct here. I just didn't see what use the caller would have for a string containing an embedded null here.

Merge new test into existing cfi.s test, remove gratuitous directives in the
round trip test. Replace use of deprecated FileCheck syntax.

I somehow managed to omit deleting the old tests in the last patchset. I think it was implied that I would, but I'm pushing up to make sure there aren't any objections (and I didn't make any mistakes).

In D91573#2403817, @dexonsmith wrote:

I suggest changing this patch to include the isValidYaml function from https://reviews.llvm.org/D84050 and call that, and then that revision can be rebased on top of this one. If you do that, this LGTM.

Remove one redundant call to vector::shrink_to_fit

I also added calls to vector::shrink_to_fit to maybe make it slightly more likely for a sanitizer without c++ STL annotations to catch out-of-bounds accesses, but if this is just noise I'm happy to remove them.

I added something along the lines of what you proposed, let me know what you
think. I also switched to a vector<uint8_t> to get more explicit control over
the terminator, and to yaml::Stream to be able to directly call validate()
such that every document is checked (in case the fuzzer produces inputs with
multiple documents).

Opened by mistake, intended to update D91545

Split up tests on combinations of addrspace, memory scope, memory ordering, and operation. I felt this was a reasonable amount of tests, but I can split things down further if needed.

In D84050#2392467, @dexonsmith wrote:

In D84050#2391741, @scott.linder wrote:

Restore the correct behavior of Scanner::scanAliasOrAnchor, last version of the patch mistakenly changed Token.Range when only intending to fix the error checking around an empty alias/anchor.

I think this version now removes the assumption that End points to a dereferencable '\0', but I'm not sure how to go about testing this comprehensively.

One idea would be to hack the parser to always duplicate the input, append a bad character (like '\1'), and parse the new buffer with the original bounds. If something reads too far it would likely get upset at the bad character. Running the test suites for LLVM and Clang with that hack might be enough coverage.

Is there a libfuzzer instance for the yaml parser? That's probably the ideal thing to do, and if it's already set up maybe it's not too hard.

I ran the fuzzer and quickly found two places I had missed. I've been running the fuzzer for about a half hour now and haven't seen another crash, so I am tentatively posting the patch. I'll leave the fuzzer running for as long as I reasonably can to try to find additional cases.