[PATCH] D65273: [llvm-objcopy] - Reimplement strip-dwo-groups.test to stop using the precompiled object.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jul 27 03:37:35 PDT 2019
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
More information about the llvm-commits
mailing list