[PATCH] [core] add support to resolve section groups.
Rui Ueyama
ruiu at google.com
Tue Mar 25 16:18:23 PDT 2014
LGTM with a few nits, but please wait for Nick's review.
================
Comment at: lib/Core/Resolver.cpp:219
@@ +218,3 @@
+ // for the first time
+ if (isFirstTime) {
+ for (const Reference *r : atom) {
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > Shankar Kalpathi Easwaran wrote:
> > > Rui Ueyama wrote:
> > > > Is this the correct semantics? If it's not a COMDAT group, we have to report an error for duplicate section group, no?
> > > No. The typeGroupComdat atom has the list of all group child members.
> > Yes, that's I know. But I don't think that answers my comment, I guess. Can you elaborate, please?
> Section groups are meant to be resolved in a way that which ever group appears first gets selected by the resolver. The second group just gets ignored. The kindGroupComdat atom models only the signature of the group. I If the symbol was non-COMDAT line 233 handles it as a regular symbol, which would detect whether the symbol was a duplicate symbol or not.
>
>
Makes sense. So if some symbol is a regular symbol in an object file, and the same symbol in a different object file is a COMDAT group symbol, something odd will happren? I think that's a bug of the program being linked, but just wondering.
================
Comment at: lib/Core/Resolver.cpp:230
@@ +229,3 @@
+ } // Iterate over references
+ } // isFirstTime
+ } else {
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > Remove comments from the above three lines. We don't usually add comments like them.
> Ok.
Comments were not removed.
================
Comment at: lib/ReaderWriter/Native/WriterNative.cpp:57
@@ -41,1 +56,3 @@
+ } // end loop iteration
+ } // end if
}
----------------
Remove comments.
http://llvm-reviews.chandlerc.com/D3182
More information about the llvm-commits
mailing list