[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 25 inline comments as done.
plotfi added inline comments.


================
Comment at: tools/llvm-objcopy/Object.h:336
+protected:
+  std::string OwnedName;
   std::vector<uint8_t> Data;
----------------
jakehehrlich wrote:
> plotfi wrote:
> > jakehehrlich wrote:
> > > So it turns out that we want to make names std::string instead of StringRefs anyway so this is no longer needed. (two other changes also needed this). That will likely be landing soon.
> > ETA?
> I think that's happening in this change: https://reviews.llvm.org/D50463
> 
> I think Paul is finished with GSoC so it's up to Paul. You can go ahead and use code from that change in this change so that the rebase will be minimal.
As far as I can tell, this is not comitted yet. Should I wait for this to land or take it from the diferential you sent?


================
Comment at: tools/llvm-objcopy/Object.h:328
 
 class OwnedDataSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
----------------
jakehehrlich wrote:
> I think you should just not change OwnedDataSection. It has a very small amount of functionality that isn't worth trying to inherit.
I was actually encouraged to inherit from OwnedDataSection in a previous review. What exactly should I do here? I need more clarity here in-order to move forward. 


================
Comment at: tools/llvm-objcopy/Object.h:349
 
+class CompressedSection : public OwnedDataSection {
+  MAKE_SEC_WRITER_FRIEND
----------------
jakehehrlich wrote:
> I think you'll find it easier to write this if you don't inherit from OwnedDataSection and instead reimplement the functionality yourself using an internal SmallVector<char, 0>. You won't need changes to OwnedDataSection and you can avoid every single compressed data copy.
This is what I had in previous diffs. Will change things back and leave OwnedDataSection the way it is. 


================
Comment at: tools/llvm-objcopy/Object.h:360
       : Data(std::begin(Data), std::end(Data)) {
-    Name = SecName;
+    OriginalData = ArrayRef<uint8_t>(Data.data(), Data.size());
+    OwnedName = SecName.str();
----------------
jakehehrlich wrote:
> Again, this is not how original data should be set. You don't need to use OwnedDataSection at all.
I think I may just not use OwnedDataSection in implementing CompressedDataSection. It wasn't like that before.


================
Comment at: tools/llvm-objcopy/Object.h:369
+public:
+  CompressedSection(const SectionBase &Sec,
+                    DebugCompressionType CompressionType)
----------------
jakehehrlich wrote:
> The Size needs to be set in the constructor. Prior to all the fancy changes that people are making now. Most Sizes could be quite trivially determined. Size is used in layout. Because I didn't foresee Size needing to be determined dynamically for different systems, it's just a single variable when it really needs to be a virtual function that takes some magical argument that will allow it to know the proper size for a given output format. The issues here haven't been fully fleshed out but I think a solution that Alex and James proposed to solve other issues and not just this one)could be made to solve this. For the time being, the best option is to use an upper bound. It sucks; I dislike it, but I don't have a better recommendation.
> 
> I should finally get the time I need to refactor this in a couple weeks. I should be able to solve a lot of these overarching issues then.
> 
> In the meantime, I think this needs to compress the data within the section so that it can get a reasonable upper bound on the size.
Oh you're saying that CompressedSection::Size should be the compressed size, not the decompressed size??? At the moment I am doing compression in the visitor. 

I still dont understand why size needs to be passed as a constructor parameter. Isn't that going to come from the Section I am passing in anyways?


================
Comment at: tools/llvm-objcopy/Object.h:377
+    Flags |= Sec.Flags;
+    if (isGnuStyle)
+      return;
----------------
jakehehrlich wrote:
> Does it cause problems with other tools if we set SHF_COMPRESSED for GNU style? If not I think I'd like to set it regardless.
it might, but I dont remember off the top of my head. I will try out setting it and using gnu objcopy to see what happens. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:294
 static bool isDebugSection(const SectionBase &Sec) {
-  return Sec.Name.startswith(".debug") || Sec.Name.startswith(".zdebug") ||
+  return !Sec.Name.find(".debug") || !Sec.Name.find(".zdebug") ||
          Sec.Name == ".gdb_index";
----------------
jakehehrlich wrote:
> I think find works from anywhere. I'd prefer if the behavior was still as it is with startswith. I think `StringRef(Sec.Name).startswith(...)` would work or in the other patch `std::equal(S2.begin(), S2.end(), S1.begin());` was used with an auxiliary StartsWith function: https://reviews.llvm.org/D50463
> 
> either way I'd just prefer we keep the 'startswith' behavior 
Ah, good stuff. Alright, I wasn't sure if you would prefer StringReg(std::string) or not. Unfortunately all the nice startswith/endswith methods for std::string are in C++20. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678