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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar 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
----------------
MaskRay wrote:
> 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.
> For example, we don't have to emphasize the previous bug

Sometimes the description of the previous bug can be helpful, but I agree with you that here it is not.
I'll remove the part that starts from "In the past..." before the commit.


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

https://reviews.llvm.org/D65273