[llvm] [yaml2obj] Don't use uninitialized Type (PR #123274)

James Henderson via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 17 01:28:43 PST 2025


jh7370 wrote:

> > I think we may have been looking at different cases, though I'm not 100% certain. When I tried this out yesterday, I omitted the `Type` field from the YAML entirely. As a result, the `getStringValue` call on line 1601 resulted in an error (because `Type` was missing) and `TypeStr` was left as an empty string. Subsequently, `Type` was then read without being initialised.
> > Looking at the code and your description, in the case you've been looking at, I'm guessing you might have a `Type` field in the YAML that starts with `SHT_` but which doesn't have a valid mapping (based on the test you described, because the machine-specific section type is used with a non-applicable machine). Is that correct?
> 
> Close, there are SHT_s which are ON/OF deppending on machine type set. But the same happens with non-existent SHT_s as well.
> 
> > Your fix only fixes the latter case, which is insufficient. The correct fix takes place outside the if statement at line 1602, in one form or another. If, as you described, it's not possible to simply do `return` from this function after seeing the invalid/missing `Type` value in the YAML, then we should adopt MaskRay's suggestion of zero-initialising the field in the first place, with an `SHT_NULL` value like you are doing already (but at the declaration point, not here). state anyway.
> 
> To clarify "zero-initialising the field", are we talking about local variable 'Type', not a field

Yes - see my edit to the original comment (looks like you were looking at the original one, which I fixed as I realised it was unclear).

> > This might be preferable to checking the error.
> 
> EC contains some error, not sure what we can do here?

I'm not following what this comment is saying? We can ignore the error, like we were before, if we have an initialized `Type` value.

See also my comment in #123280 - perhaps the fix is to do something with `mapRequired` itself, though I'm not sure that would necessarily be sufficient in this case.

https://github.com/llvm/llvm-project/pull/123274


More information about the llvm-commits mailing list