[YAML] Init local var not set by some branches

[vitalybuka]

It will not be set if:

1. (TypeStr.starts_with("SHT_") || isInteger(TypeStr)) == false: here we want go to switch default.
2. IO.mapRequired("Type", Type); fail parsing. It sets error internally, so probably not important what happen next, so it's go to the switch

[llvmbot]

@llvm/pr-subscribers-objectyaml

Author: Vitaly Buka (vitalybuka)

Changes

---

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

1 Files Affected:

• (modified) llvm/lib/ObjectYAML/ELFYAML.cpp (+1-1)

diff --git a/llvm/lib/ObjectYAML/ELFYAML.cpp b/llvm/lib/ObjectYAML/ELFYAML.cpp
index 7e94d01a971534..83e6cf76dd746f 100644
--- a/llvm/lib/ObjectYAML/ELFYAML.cpp
+++ b/llvm/lib/ObjectYAML/ELFYAML.cpp
@@ -1588,7 +1588,7 @@ static bool isInteger(StringRef Val) {
 
 void MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::mapping(
     IO &IO, std::unique_ptr<ELFYAML::Chunk> &Section) {
-  ELFYAML::ELF_SHT Type;
+  ELFYAML::ELF_SHT Type = ELF::ET_NONE;
   StringRef TypeStr;
   if (IO.outputting()) {
     if (auto *S = dyn_cast<ELFYAML::Section>(Section.get()))

[fmayer]

I feel like we may be hiding another bug here.. on line 1603 Type is never initialized AFAICT, so that is clearly incorrect.

Nevermind, that is an output parameter. It also gets set on hat branch

[fmayer]

OK, so I looked more. I think only on one branch this is not set, that is line 1601 if the condition below is false.

For 1597 branch we should bail out in the 1613 condition below.

[vitalybuka]

OK, so I looked more. I think only on one branch this is not set, that is line 1601 if the condition below is false.

For 1597 branch we should bail out in the 1613 condition below.

it will not be set if:

1. (TypeStr.starts_with("SHT_") || isInteger(TypeStr)) == false
   here we want go to switch default.
2. IO.mapRequired("Type", Type); fail parsing
   it sets error internally, so probably not important what happen next

[fmayer]

nit: "Init local set by a single branche" typo and not sure what that is trying to tell me

[vitalybuka]

nit: "Init local set by a single branche" typo and not sure what that is trying to tell me

PTAL

[jh7370]

This change is incorrect. For starters, ELF_SHT is a section header type, but you're initialising it with an ELF type, which is an orthogonal concept.

I do agree that there is an unitialised variable usage. However, I don't think initialising the value like you have (even if it were a valid value to use) is correct. Please revert this PR and create a new one with the correct fix. (If necessary, I'll revert this PR, but would prefer it if the original contributor/reviewer did).

The "Type" field in the YAML is a mandatory field for sections. It doesn't make sense to try to parse the rest of the Section block if it doesn't have one, because the contents are meaningless. Instead, if the field has been found as unset when calling getStringValue at line 1601 in this PR, we should bail out of the code. This could be detected either by TypeStr being checked for being empty, or by checking the value of (I think) IO.EC. I think I'd prefer the former, because this would mean (valid) section parsing isn't aborted because of unrelated errors elsewhere in the document.

[vitalybuka]

This change is incorrect. For starters, ELF_SHT is a section header type, but you're initialising it with an ELF type, which is an orthogonal concept.

I am sorry, I didn't realize this. I thought fallback to default is intentional.

I do agree that there is an unitialised variable usage. However, I don't think initialising the value like you have (even if it were a valid value to use) is correct. Please revert this PR and create a new one with the correct fix. (If necessary, I'll revert this PR, but would prefer it if the original contributor/reviewer did).

Will do.

The "Type" field in the YAML is a mandatory field for sections. It doesn't make sense to try to parse the rest of the Section block if it doesn't have one, because the contents are meaningless. Instead, if the field has been found as unset when calling getStringValue at line 1601 in this PR, we should bail out of the code. This could be detected either by TypeStr being checked for being empty, or by checking the value of (I think) IO.EC. I think I'd prefer the former, because this would mean (valid) section parsing isn't aborted because of unrelated errors elsewhere in the document.

Thanks for the advice!