[PATCH] D65273: [llvm-objcopy] - Reimplement strip-dwo-groups.test to stop using the precompiled object.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-dwo-groups.test:5
 
-// Source code of groups.o:
-//
-// template <class T>
-// struct S {
-//    static constexpr T X = T(1);
-//    T getX() { return X; }
-// };
-// void f() {
-//  S<int> A;
-//  S<double> B;
-//  int a = A.getX();
-//  int b = B.getX();
-// }
-//
-// clang -g -gsplit-dwarf -std=c++11 -c groups.cpp -o groups.o
+## `llvm-objcopy --strip-dwo` strips out dwo sections, as a result, the index of 
+## the symbol table, the indices of the symbols and the indices of the sections
----------------
grimar wrote:
> MaskRay wrote:
> > The comment may be too verbose. Suggested wording (please improve as you rewrite :) ):
> > 
> > After the removal of .dwo sections, the indices of sections which go after them will change. Check that the sh_link and sh_info fields of .group and their contents (indices of member sections) are updated.
> Your version looks OK to me.
> 
> But I do not feel that "too verbose" is a problem,
> I like to read the description of the problem and the details.
> Unfortunately often svn/git commit comments are the only source.
> "too verbose" is in my understanding can be used for the code comments,
> but while we are in the test cases area, If you do not mind, I would keep it verbose.
> Unfortunately often svn/git commit comments are the only source.

Agreed.

> "too verbose" is in my understanding can be used for the code comments, but while we are in the test cases area, If you do not mind, I would keep it verbose.

I am fine with the current version but I feel we can convey the same information while making it more concise. For example, we don't have to emphasize the previous bug:

> In the past `llvm-objcopy --strip-dwo` used to produce invalid binaries with broken .group section, this test verifies the correctness of Link, Info and the content of this section.

The expected behavior is kind of self-explanatory with the existence of the test.

> their contents (indices of member sections) are updated.

It emphasizes the purpose updating the contents.


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

https://reviews.llvm.org/D65273