Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YAML] Don't validate Fill::Size after error #123280

Merged

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Jan 17, 2025

Size is required, so we don't know if it's in
uninitialized state after the previous error.

Triggers msan on llvm/test/tools/yaml2obj/ELF/custom-fill.yaml NOSIZE test.

We have Fill Section with Pattern, but no size. Before the fix it produced error:

YAML:169:5: error: missing required key 'Size'
  - Type:    Fill
    ^
YAML:169:5: error: "Size" can't be 0 when "Pattern" is not empty
  - Type:    Fill

The same applies to MachOYAML::Section fields content and size.
However MachOYAML::Section matches size first, so on error,
content is not set anyway. Added error checking just in case.

@llvmbot
Copy link
Member

llvmbot commented Jan 17, 2025

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-objectyaml

Author: Vitaly Buka (vitalybuka)

Changes

Size is required, so we don't know if it's in
uninitialized state after the previous error.

Triggers msan on llvm/test/tools/yaml2obj/ELF/custom-fill.yaml


Full diff: https://github.com/llvm/llvm-project/pull/123280.diff

1 Files Affected:

  • (modified) llvm/lib/ObjectYAML/ELFYAML.cpp (+3-1)
diff --git a/llvm/lib/ObjectYAML/ELFYAML.cpp b/llvm/lib/ObjectYAML/ELFYAML.cpp
index e0e941cff94c52..1d52cdef79400f 100644
--- a/llvm/lib/ObjectYAML/ELFYAML.cpp
+++ b/llvm/lib/ObjectYAML/ELFYAML.cpp
@@ -1750,7 +1750,9 @@ void MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::mapping(
 std::string MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::validate(
     IO &io, std::unique_ptr<ELFYAML::Chunk> &C) {
   if (const auto *F = dyn_cast<ELFYAML::Fill>(C.get())) {
-    if (F->Pattern && F->Pattern->binary_size() != 0 && !F->Size)
+    // Can't check the `Size`, as it's required and may be left uninitialized by
+    // previous error.
+    if (!io.error() && F->Pattern && F->Pattern->binary_size() != 0 && !F->Size)
       return "\"Size\" can't be 0 when \"Pattern\" is not empty";
     return "";
   }

@vitalybuka vitalybuka requested a review from MaskRay January 17, 2025 04:11
llvm/lib/ObjectYAML/ELFYAML.cpp Show resolved Hide resolved
@vitalybuka vitalybuka requested a review from MaskRay January 18, 2025 02:23
@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.yaml-dont-validate-fillsize-after-error to users/vitalybuka/spr/nfcyaml-add-ioerror January 18, 2025 20:31
llvm/test/ObjectYAML/MachO/section_data.yaml Outdated Show resolved Hide resolved
llvm/test/tools/yaml2obj/ELF/custom-fill.yaml Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/ELFYAML.cpp Show resolved Hide resolved
@vitalybuka vitalybuka requested a review from jh7370 January 21, 2025 08:26
vitalybuka added a commit that referenced this pull request Jan 21, 2025
Size is required, so we don't know if it's in
uninitialized state after the previous error.

Triggers msan on llvm/test/tools/yaml2obj/ELF/custom-fill.yaml

Pull Request: #123280
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/yaml-dont-validate-fillsize-after-error branch from 7c9e807 to 0caa47f Compare January 21, 2025 08:33
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/nfcyaml-add-ioerror branch from 487784c to e696bc4 Compare January 21, 2025 08:34
Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but might want to get @MaskRay to give a thumbs up to, given he's made a comment.

llvm/test/ObjectYAML/MachO/section_data.yaml Outdated Show resolved Hide resolved
Size is required, so we don't know if it's in
uninitialized state after the previous error.

Triggers msan on llvm/test/tools/yaml2obj/ELF/custom-fill.yaml

Pull Request: #123280
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/yaml-dont-validate-fillsize-after-error branch from 4ddbe28 to c6e72c4 Compare January 22, 2025 19:18
@vitalybuka vitalybuka force-pushed the users/vitalybuka/spr/nfcyaml-add-ioerror branch from 0a895fd to c6e72c4 Compare January 22, 2025 19:19
vitalybuka added a commit that referenced this pull request Jan 23, 2025
Base automatically changed from users/vitalybuka/spr/nfcyaml-add-ioerror to main January 23, 2025 11:56
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit 66e49e3 into main Jan 23, 2025
5 of 6 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/yaml-dont-validate-fillsize-after-error branch January 23, 2025 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants