[PATCH] D88288: [Archive] Don't throw away errors for malformed archive members

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht requested changes to this revision.
rupprecht added a comment.
This revision now requires changes to proceed.

Requesting changes mostly because this is marked as accepted but I still see build errors; overall this looks fine though.



================
Comment at: llvm/lib/Object/SymbolicFile.cpp:47-48
+
   switch (Type) {
   case file_magic::bitcode:
     if (Context)
----------------
I'm still seeing `-Wswitch` errors:

```
/home/rupprecht/src/llvm-project/llvm/lib/Object/SymbolicFile.cpp:47:11: warning: 8 enumeration values not handled in switch: 'unknown', 'archive', 'macho_universal_binary'... [-Wswitch]
  switch (Type) {
```


================
Comment at: llvm/lib/Object/SymbolicFile.cpp:51
       return IRObjectFile::create(Object, *Context);
-    LLVM_FALLTHROUGH;
-  case file_magic::unknown:
-  case file_magic::archive:
-  case file_magic::coff_cl_gl_object:
-  case file_magic::macho_universal_binary:
-  case file_magic::windows_resource:
-  case file_magic::pdb:
-  case file_magic::minidump:
-  case file_magic::tapi_file:
     return errorCodeToError(object_error::invalid_file_type);
   case file_magic::elf:
----------------
MaskRay wrote:
> grimar wrote:
> > jhenderson wrote:
> > > grimar wrote:
> > > > I think the `Content` is always non-null here now?
> > > Yeah, I think this can be simplified. I'm not going to have a chance to update the patch again today, but will fix in the commit, if you're otherwise happy.
> > > 
> > > It will become:
> > > ```
> > >   case file_magic::bitcode:
> > >     return IRObjectFile::create(Object, *Context);
> > > ```
> > The rest looks fine to me. Please wait to see @MaskRay/@rupprecht opinions too
> `Context` is always non-null. This is however a hidden fact only clear after I read all the callers..
I found this thread hard to reconcile with the code: IIUC, `Context` is only non-null *in this case statement*: if `Context` is null and this is a bitcode file, `isSymbolicFile` returns false, and an error is returned above, so we don't reach this line. **However**, if this is a wasm_object (just one example), `isSymbolicFile` returns true even if `Context` is null. `Context` can be null in this switch statement, but not this particular case.

I think it's fine to make that simplification, but with a comment that `isSymbolicFile` verifies that `Context` is non-null when the type is bitcode. Otherwise, the nullness of `Context` is very difficult to reason with -- the baseline is much easier to understand in that regard.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88288/new/

https://reviews.llvm.org/D88288