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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl 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
----------------
jhenderson wrote:
> 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?
Done. Thanks for the suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121346