Page MenuHomePhabricator

scott.linder (Scott Linder)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 15 2018, 8:31 AM (157 w, 6 d)

Recent Activity

Fri, Jan 22

scott.linder added inline comments to D95241: AMDGPU: Fix redundant FP spilling/assert in some functions.
Fri, Jan 22, 10:18 AM · Restricted Project

Thu, Jan 21

scott.linder accepted D95071: [AMDGPU] Fix the inconsistency in soffset for MUBUF stack accesses..

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

Thu, Jan 21, 9:01 AM · Restricted Project
scott.linder accepted D94801: [AMDGPU] Test case demonstrating issues with generation of .debug_frame.
Thu, Jan 21, 8:35 AM · Restricted Project

Wed, Jan 20

scott.linder added a comment to D95071: [AMDGPU] Fix the inconsistency in soffset for MUBUF stack accesses..

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

Wed, Jan 20, 3:03 PM · Restricted Project
scott.linder added a comment to D95071: [AMDGPU] Fix the inconsistency in soffset for MUBUF stack accesses..

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:

Wed, Jan 20, 2:27 PM · Restricted Project

Sun, Jan 17

scott.linder added inline comments to D83495: [DebugInfo] Add DWARF emission for DBG_VALUE_LIST.
Sun, Jan 17, 12:18 PM · Restricted Project, debug-info

Wed, Jan 13

scott.linder added inline comments to D83495: [DebugInfo] Add DWARF emission for DBG_VALUE_LIST.
Wed, Jan 13, 12:31 PM · Restricted Project, debug-info
scott.linder added inline comments to D83495: [DebugInfo] Add DWARF emission for DBG_VALUE_LIST.
Wed, Jan 13, 9:51 AM · Restricted Project, debug-info

Mon, Jan 11

scott.linder committed rGc15b0e2229ea: [Clang][Docs] Fix ambiguity in clang-offload-bundler docs (authored by scott.linder).
[Clang][Docs] Fix ambiguity in clang-offload-bundler docs
Mon, Jan 11, 9:24 AM
scott.linder closed D94338: [Clang][Docs] Fix ambiguity in clang-offload-bundler docs.
Mon, Jan 11, 9:24 AM · Restricted Project

Fri, Jan 8

scott.linder added reviewers for D94338: [Clang][Docs] Fix ambiguity in clang-offload-bundler docs: t-tye, yaxunl, kzhuravl.
Fri, Jan 8, 1:57 PM · Restricted Project
scott.linder requested review of D94338: [Clang][Docs] Fix ambiguity in clang-offload-bundler docs.
Fri, Jan 8, 1:56 PM · Restricted Project

Dec 23 2020

scott.linder added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 23 2020, 1:28 PM · Restricted Project
scott.linder added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 23 2020, 11:37 AM · Restricted Project

Dec 22 2020

scott.linder added inline comments to D83495: [DebugInfo] Add DWARF emission for DBG_VALUE_LIST.
Dec 22 2020, 3:27 PM · Restricted Project, debug-info
scott.linder added inline comments to D92578: [DebugInfo] Handle DBG_VALUES with multiple variable location operands in MIR.
Dec 22 2020, 3:24 PM · Restricted Project, debug-info
scott.linder added a comment to D82363: [DebugInfo] Add new instruction and expression operator for variadic debug values.

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.

Dec 22 2020, 3:20 PM · Restricted Project, debug-info
scott.linder added reviewers for D93721: [NFC][AMDGPU] Do not override GetDefaultDwarfVersion: t-tye, laurentm0, yaxunl.

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.

Dec 22 2020, 11:28 AM · Restricted Project
scott.linder requested review of D93721: [NFC][AMDGPU] Do not override GetDefaultDwarfVersion.
Dec 22 2020, 11:26 AM · Restricted Project
scott.linder added inline comments to D92483: AMDGPU - Use MUBUF instructions for global address space access.
Dec 22 2020, 10:08 AM · Restricted Project

Dec 21 2020

scott.linder committed rGffba47df7646: Revert "[AMDGPU][HIP] Switch default DWARF version to 5" (authored by scott.linder).
Revert "[AMDGPU][HIP] Switch default DWARF version to 5"
Dec 21 2020, 1:44 PM
scott.linder added a reverting change for rGc4d10e7e9bb4: [AMDGPU][HIP] Switch default DWARF version to 5: rGffba47df7646: Revert "[AMDGPU][HIP] Switch default DWARF version to 5".
Dec 21 2020, 1:44 PM
scott.linder added a reverting change for D89484: [AMDGPU][HIP] Switch default DWARF version to 5: rGffba47df7646: Revert "[AMDGPU][HIP] Switch default DWARF version to 5".
Dec 21 2020, 1:44 PM · Restricted Project
scott.linder closed D93648: Revert "[AMDGPU][HIP] Switch default DWARF version to 5".
Dec 21 2020, 1:44 PM · Restricted Project
scott.linder added reviewers for D93648: Revert "[AMDGPU][HIP] Switch default DWARF version to 5": kzhuravl, t-tye.
Dec 21 2020, 9:55 AM · Restricted Project
scott.linder added a reverting change for rGc4d10e7e9bb4: [AMDGPU][HIP] Switch default DWARF version to 5: D93648: Revert "[AMDGPU][HIP] Switch default DWARF version to 5".
Dec 21 2020, 9:55 AM
scott.linder requested review of D93648: Revert "[AMDGPU][HIP] Switch default DWARF version to 5".
Dec 21 2020, 9:55 AM · Restricted Project
scott.linder added a reverting change for D89484: [AMDGPU][HIP] Switch default DWARF version to 5: D93648: Revert "[AMDGPU][HIP] Switch default DWARF version to 5".
Dec 21 2020, 9:55 AM · Restricted Project
scott.linder added a comment to D92483: AMDGPU - Use MUBUF instructions for global address space access.

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.

Dec 21 2020, 9:10 AM · Restricted Project

Dec 17 2020

scott.linder added inline comments to D76519: [NFC] Refactor how CFI move sections are represented in AsmPrinter.
Dec 17 2020, 11:41 AM · Restricted Project
scott.linder added inline comments to D76519: [NFC] Refactor how CFI move sections are represented in AsmPrinter.
Dec 17 2020, 11:40 AM · Restricted Project

Dec 16 2020

scott.linder added inline comments to D93403: AMDGPU: Remove SGPRSpillVGPRDefinedSet hack.
Dec 16 2020, 9:48 AM · Restricted Project

Dec 15 2020

scott.linder accepted D92718: [CMake] Avoid __FakeVCSRevision.h with no git repository.

LGTM, thank you!

Dec 15 2020, 2:08 PM · Restricted Project
scott.linder updated subscribers of D93211: Re-apply "[CMake][compiler-rt][AArch64] Avoid preprocessing LSE builtins separately".

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.

Dec 15 2020, 11:40 AM · Restricted Project
scott.linder added a comment to D92718: [CMake] Avoid __FakeVCSRevision.h with no git repository.

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:

Dec 15 2020, 11:03 AM · Restricted Project

Dec 14 2020

scott.linder accepted D93234: [NFC] Remove trailing whitespace in llvm/CMakeLists.txt.
Dec 14 2020, 9:49 AM · Restricted Project
scott.linder added inline comments to D76519: [NFC] Refactor how CFI move sections are represented in AsmPrinter.
Dec 14 2020, 6:57 AM · Restricted Project

Dec 11 2020

scott.linder added a comment to D76519: [NFC] Refactor how CFI move sections are represented in AsmPrinter.

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?

Dec 11 2020, 2:36 PM · Restricted Project
scott.linder added a comment to D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.

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.

Dec 11 2020, 2:02 PM · debug-info, Restricted Project
scott.linder added reviewers for D76519: [NFC] Refactor how CFI move sections are represented in AsmPrinter: dblaikie, MaskRay.
Dec 11 2020, 12:58 PM · Restricted Project
scott.linder added a comment to D93069: [SmallVector] Copy new docs into Doxygen comment.

Thanks!

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

Dec 11 2020, 11:38 AM · Restricted Project
scott.linder committed rG32910f780df4: [SmallVector][NFC] Link to ProgrammersManual from SmallVector docs (authored by scott.linder).
[SmallVector][NFC] Link to ProgrammersManual from SmallVector docs
Dec 11 2020, 11:37 AM
scott.linder added a comment to D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.

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.

Dec 11 2020, 11:25 AM · debug-info, Restricted Project
scott.linder added a comment to D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.

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.

Dec 11 2020, 11:21 AM · debug-info, Restricted Project
scott.linder added a comment to D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.

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?

Dec 11 2020, 10:03 AM · debug-info, Restricted Project
scott.linder updated the diff for D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.

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

Dec 11 2020, 9:53 AM · debug-info, Restricted Project

Dec 10 2020

scott.linder committed rG10b5eaed917d: [SmallVector] Copy new docs into Doxygen comment (authored by scott.linder).
[SmallVector] Copy new docs into Doxygen comment
Dec 10 2020, 2:21 PM
scott.linder closed D93069: [SmallVector] Copy new docs into Doxygen comment.
Dec 10 2020, 2:20 PM · Restricted Project
scott.linder added reviewers for D93069: [SmallVector] Copy new docs into Doxygen comment: lattner, dexonsmith, rnk, mehdi_amini, dblaikie.

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

Dec 10 2020, 2:00 PM · Restricted Project
scott.linder requested review of D93069: [SmallVector] Copy new docs into Doxygen comment.
Dec 10 2020, 1:58 PM · Restricted Project
scott.linder added a comment to D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.

(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)

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)

Dec 10 2020, 7:27 AM · debug-info, Restricted Project

Dec 9 2020

scott.linder committed rG19c56e11fa48: [MC] Fix ICE with non-newline terminated input (authored by scott.linder).
[MC] Fix ICE with non-newline terminated input
Dec 9 2020, 3:40 PM
scott.linder closed D92681: [MC] Fix ICE with non-newline terminated input.
Dec 9 2020, 3:40 PM · Restricted Project
scott.linder updated the diff for D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.

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

Dec 9 2020, 3:09 PM · debug-info, Restricted Project
scott.linder committed rG9260a999990c: [MC][AMDGPU] Consume EndOfStatement in asm parser (authored by scott.linder).
[MC][AMDGPU] Consume EndOfStatement in asm parser
Dec 9 2020, 1:46 PM
scott.linder closed D92690: [MC][AMDGPU] Consume EndOfStatement in asm parser.
Dec 9 2020, 1:46 PM · Restricted Project
scott.linder updated the diff for D92690: [MC][AMDGPU] Consume EndOfStatement in asm parser.

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.

Dec 9 2020, 1:41 PM · Restricted Project
scott.linder committed rGf5f4b8b60fc0: [AMDGPU][MC] Restore old error position for "too few operands" (authored by scott.linder).
[AMDGPU][MC] Restore old error position for "too few operands"
Dec 9 2020, 1:10 PM
scott.linder closed D92960: [AMDGPU][MC] Restore old error position for "too few operands".
Dec 9 2020, 1:10 PM · Restricted Project
scott.linder added a comment to D90345: [DebugInfo] Fix ICE in DwarfCompileUnit::constructSubprogramScopeDIE.

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.

Dec 9 2020, 12:43 PM · Restricted Project
scott.linder added reviewers for D92960: [AMDGPU][MC] Restore old error position for "too few operands": dp, rampitec.
Dec 9 2020, 12:20 PM · Restricted Project
scott.linder requested review of D92960: [AMDGPU][MC] Restore old error position for "too few operands".
Dec 9 2020, 12:20 PM · Restricted Project
scott.linder added a comment to D92690: [MC][AMDGPU] Consume EndOfStatement in asm parser.
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 :-)

Dec 9 2020, 11:19 AM · Restricted Project

Dec 8 2020

scott.linder updated the diff for D92690: [MC][AMDGPU] Consume EndOfStatement in asm parser.

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

Dec 8 2020, 3:09 PM · Restricted Project

Dec 4 2020

scott.linder added inline comments to D92617: [DWARF] Allow toolchain to adjust specified DWARF version..
Dec 4 2020, 2:34 PM · Restricted Project
scott.linder committed rGd55d6806ad72: [MC] Consume EndOfStatement in .cfi_{sections,endproc} (authored by scott.linder).
[MC] Consume EndOfStatement in .cfi_{sections,endproc}
Dec 4 2020, 2:31 PM
scott.linder closed D92612: [MC] Consume EndOfStatement in .cfi_{sections,endproc}.
Dec 4 2020, 2:31 PM · Restricted Project
scott.linder added reviewers for D92690: [MC][AMDGPU] Consume EndOfStatement in asm parser: MaskRay, foad, rampitec, arsenm, dp.
Dec 4 2020, 2:13 PM · Restricted Project
scott.linder requested review of D92690: [MC][AMDGPU] Consume EndOfStatement in asm parser.
Dec 4 2020, 2:11 PM · Restricted Project
scott.linder added inline comments to D92617: [DWARF] Allow toolchain to adjust specified DWARF version..
Dec 4 2020, 1:12 PM · Restricted Project
scott.linder added a comment to D92681: [MC] Fix ICE with non-newline terminated input.

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.

Dec 4 2020, 12:57 PM · Restricted Project
scott.linder added reviewers for D92681: [MC] Fix ICE with non-newline terminated input: grosbach, MaskRay, niravd, eliben, espindola.
Dec 4 2020, 12:56 PM · Restricted Project
scott.linder requested review of D92681: [MC] Fix ICE with non-newline terminated input.
Dec 4 2020, 12:54 PM · Restricted Project
scott.linder added inline comments to D92612: [MC] Consume EndOfStatement in .cfi_{sections,endproc}.
Dec 4 2020, 12:03 PM · Restricted Project
scott.linder updated the diff for D92612: [MC] Consume EndOfStatement in .cfi_{sections,endproc}.

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

Dec 4 2020, 12:02 PM · Restricted Project
scott.linder added inline comments to D92612: [MC] Consume EndOfStatement in .cfi_{sections,endproc}.
Dec 4 2020, 7:27 AM · Restricted Project

Dec 3 2020

scott.linder added reviewers for D92612: [MC] Consume EndOfStatement in .cfi_{sections,endproc}: grosbach, MaskRay, niravd, eliben.
Dec 3 2020, 3:01 PM · Restricted Project
scott.linder requested review of D92612: [MC] Consume EndOfStatement in .cfi_{sections,endproc}.
Dec 3 2020, 2:57 PM · Restricted Project
scott.linder committed rGf6b9afae00d6: [AMDGPU] Extend and reorganize memory legalizer tests (authored by scott.linder).
[AMDGPU] Extend and reorganize memory legalizer tests
Dec 3 2020, 11:37 AM
scott.linder closed D91545: [AMDGPU] Extend and reorganize memory legalizer tests.
Dec 3 2020, 11:37 AM · Restricted Project

Nov 20 2020

scott.linder updated the diff for D91545: [AMDGPU] Extend and reorganize memory legalizer tests.

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).

Nov 20 2020, 2:30 PM · Restricted Project

Nov 18 2020

scott.linder added a comment to D91573: [YAMLIO] Add a generic YAML fuzzer harness.

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.

Nov 18 2020, 3:08 PM · Restricted Project
scott.linder committed rG2980933d850b: [YAMLIO] Support non-null-terminated inputs (authored by scott.linder).
[YAMLIO] Support non-null-terminated inputs
Nov 18 2020, 3:06 PM
scott.linder committed rG544cb649d778: [YAMLIO] Add a generic YAML fuzzer harness (authored by scott.linder).
[YAMLIO] Add a generic YAML fuzzer harness
Nov 18 2020, 3:06 PM
scott.linder closed D91573: [YAMLIO] Add a generic YAML fuzzer harness.
Nov 18 2020, 3:06 PM · Restricted Project
scott.linder closed D84050: [YAMLIO] Support non-null-terminated inputs.
Nov 18 2020, 3:06 PM · Restricted Project
scott.linder committed rG0fe4b8e4b5b3: [NFC][AMDGPU] Remove some generic pointers in memory-legalizer tests (authored by scott.linder).
[NFC][AMDGPU] Remove some generic pointers in memory-legalizer tests
Nov 18 2020, 12:53 PM
scott.linder closed D91666: [NFC][AMDGPU] Remove some generic pointers in memory-legalizer tests.
Nov 18 2020, 12:53 PM · Restricted Project
scott.linder updated the diff for D84050: [YAMLIO] Support non-null-terminated inputs.

Remove one redundant call to vector::shrink_to_fit

Nov 18 2020, 12:51 PM · Restricted Project
scott.linder added a comment to D84050: [YAMLIO] Support non-null-terminated inputs.

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.

Nov 18 2020, 12:39 PM · Restricted Project
scott.linder updated the diff for D84050: [YAMLIO] Support non-null-terminated inputs.

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).

Nov 18 2020, 12:24 PM · Restricted Project

Nov 17 2020

scott.linder added reviewers for D91666: [NFC][AMDGPU] Remove some generic pointers in memory-legalizer tests: rampitec, t-tye, arsenm, kzhuravl, nhaehnle.
Nov 17 2020, 4:27 PM · Restricted Project
scott.linder abandoned D91667: [AMDGPU] Extend and reorganize memory legalizer tests.

Opened by mistake, intended to update D91545

Nov 17 2020, 4:26 PM · Restricted Project
scott.linder updated the diff for D91545: [AMDGPU] Extend and reorganize memory legalizer tests.

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.

Nov 17 2020, 4:26 PM · Restricted Project
scott.linder requested review of D91667: [AMDGPU] Extend and reorganize memory legalizer tests.
Nov 17 2020, 4:23 PM · Restricted Project
scott.linder requested review of D91666: [NFC][AMDGPU] Remove some generic pointers in memory-legalizer tests.
Nov 17 2020, 4:23 PM · Restricted Project
scott.linder accepted D91458: [NFC][AMDGPU] Document kernel descriptor.
Nov 17 2020, 3:14 PM · Restricted Project

Nov 16 2020

scott.linder added a comment to D84050: [YAMLIO] Support non-null-terminated inputs.

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.

Nov 16 2020, 3:32 PM · Restricted Project
scott.linder updated the summary of D91573: [YAMLIO] Add a generic YAML fuzzer harness.
Nov 16 2020, 3:24 PM · Restricted Project
scott.linder updated the diff for D84050: [YAMLIO] Support non-null-terminated inputs.

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.

Nov 16 2020, 3:20 PM · Restricted Project