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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D88288#2295523 <https://reviews.llvm.org/D88288#2295523>, @MaskRay wrote:

> Checked lld/test/ELF updates, GNU ar silently allows invalid 'symbolic' files. I think being more rigid in llvm-ar is fine.
>
> An alternative to the new `isSymbolicFile` is by checking whether `createSymbolicFile` returns an ECError whose error_code (`ECError::convertToErrorCode`) is `object_error::invalid_file_type`.

That's what I originally did, but a) the code is not particularly clean, and b) it's brittle to changes in how the error is reported (relies on on the error code being set, rather than e.g. changing to a normal LLVM error). @grimar previously suggested to change this (see the inline comment).



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:499-501
+          getSymbols(Buf, SymNames, HasObject);
+      if (auto E = SymbolsOrErr.takeError())
+        return std::move(E);
----------------
rupprecht wrote:
> nit: looks like tab characters were inserted, can you run clang-format?
That's an unfortunate UI issue in Phabricator that was recently introduced. Code that has been indented now shows up this way. There are no tab characters here.


================
Comment at: llvm/lib/Object/SymbolicFile.cpp:47
+
   switch (Type) {
   case file_magic::bitcode:
----------------
rupprecht wrote:
> Splitting up the switch leads to `-Wswitch` errors because now each switch statement is incomplete:
> 
> ```
> /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) {
>           ^
> /home/rupprecht/src/llvm-project/llvm/lib/Object/SymbolicFile.cpp:97:11: warning: 23 enumeration values not handled in switch: 'bitcode', 'elf', 'elf_relocatable'... [-Wswitch]
>   switch (Type) {
>           ^
> ```
> 
> Normally the right fix is to add a default switch case to handle all the unused cases. If you're concerned about the list of `file_magic::unknown, file_magic::archive, ...` in each method going out of sync in the two definitions (and new enums accidentally being handled with the default case), you could define that list in a macro, e.g.
> 
> ```
> #define NOT_SYMBOLIC_FILES \
>   case file_magic::unknown: \
>   case file_magic::archive: \
>   case file_magic::coff_cl_gl_object: \
> ...
>   case file_magic::tapi_file
> 
>   switch (Type) {
>     NOT_SYMBOLIC_FILES:
>       return false;
>     default:
>       return true;
>   }
> ```
> 
> (note no trailing `:` in the last case you can write `NOT_SYMBOLIC_FILES:` and not confused IDEs)
I don't particularly like macros. They are a pain to debug and not particularly readable, in my opinion. Maybe the way to resolve this is to flip the switch in `isSymbolicFile` on its head, so that any new file_magic types will return false (specified via a `default` label), until this function is modified. This means you wouldn't be able to create a symbolic file for them, which I don't think is unreasonable. This switch here then gains a `default: llvm_unreachable()` line, since it would only be reachable if the `isSymbolicFile` method returned true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88288