[PATCH] D121346: [Propeller] Encode address offsets of basic blocks relative to the end of the previous basic blocks.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ELF.cpp:670-671
+      if (Version > 1)
+        return createError("unsupported LLVM_BB_ADDR_MAP version: " +
+                           Twine(static_cast<int>(Version)));
+      Data.getU8(Cur); // Feature byte
----------------
rahmanl wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > Test case?
> > > 
> > > Also, the type is "SHT_LLVM_BB_ADDR_MAP", so probably wants to include the SHT_ too, to match (and be consistent with other error messages)
> > Looks like there's still no test case?
> Sorry, my response wasn't sent: I can't add a test to exercise this because I can't make a valid Yaml with an unsupported version number (`ELFEmitter.cpp` returns error if I specify version> 1), but I also don't think it's a good idea to remove that error handling. What do you suggest?
Hmm, good point. What do you think about the following proposal:

1) Emit a warning rather than an error with yaml2obj.
2) In this case, treat it as the max supported version (i.e. 1) and generate data like that, except with a value 2 for the Version field.

YAML is really only used for testing, so emitting an error blocks us from testing the actual production code we want to test, which seems unfortunate!

The alternative approach would be to use assembly, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121346