[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 a comment.

Has the spec for this been finalised anywhere? My main conceren is the use of section names to have semantic importance. ELF generally tries to avoid this, hence the use of section types, and it would be a shame to introduce this approach when there are other options. It would be far more preferable to include the version number in the section data somewhere, a bit like how most DWARF sections are encoded. I can think of one other possible way of doing this: change the section type value for version 1 and upwards, and rename the original value to something like SHT_LLVM_BB_ADDR_MAP_V0. Add the version field as the first N bytes (2 or 4 probably) of the new section type. Parsers understanding the old data structure only won't recognise the new section type as a recognised format. This is good because it doesn't mislead people by printing incorrect offsets (in addition to not needing to rely on the section name).



================
Comment at: llvm/docs/Extensions.rst:404-413
+a BB address map table for every function.
+
+This feature provides backward compatibility to allow reading older versions of
+the BB address map generated by older compilers.
+The section name will include a version suffix (`.v#{version-number}`) which
+specifies the version to use. The follwoing versioning schemes are currently
+supported.
----------------



================
Comment at: llvm/docs/Extensions.rst:431-432
 
-This creates a BB address map table for a function with two basic blocks.
+Version 0: basic block address offsets are computed relative to the function
+address.
+
----------------
Do we need a note saying that v0 BB Addr maps may not have the version suffix in the section name?


================
Comment at: llvm/lib/Object/ELF.cpp:640
+  // TODO: Report error in this case when version 0 becomes obsolete.
+  int Version = 0;
+  if (!VersionStr.empty() && VersionStr.startswith("v")) {
----------------
`int` seems like an odd type for `Version`. It probably should be some unsigned type?


================
Comment at: llvm/lib/Object/ELF.cpp:643-647
+      return createError("Unable to parse bb-address-map version suffix: " +
+                         VersionStr);
+    if (Version > 1)
+      return createError("Unsupported bb-address-map version: " +
+                         Twine(Version));
----------------
Coding standards say to use lower-case for first letter of error messages.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:75
+# V0-NEXT:            Offset: 0xC
+# V1-NEXT:            Offset: 0x1F
+# CHECK-NEXT:         Size: 0xD
----------------
For V1 output, I feel like it would be useful to have both the raw offset and the calculated offset printed. I'm not sure exactly what would be the best way of doing that though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121346