Page MenuHomePhabricator

[Object] Add the method for checking if a section is a debug section
ClosedPublic

Authored by djtodoro on Mar 17 2020, 4:37 AM.

Diff Detail

Event Timeline

djtodoro created this revision.Mar 17 2020, 4:37 AM
jhenderson added inline comments.Mar 17 2020, 6:40 AM
llvm/lib/Object/COFFObjectFile.cpp
332

Is this definitely correct for COFF?

llvm/lib/Object/ObjectFile.cpp
95

This looks to me like an infinite loop?

Maybe this function should just return false?

djtodoro marked 2 inline comments as done.Mar 17 2020, 8:09 AM
djtodoro added inline comments.
llvm/lib/Object/COFFObjectFile.cpp
332

As far as I can see in the llvm-objcopy tool and on some web pages, but I have not found an official document for that.

llvm/lib/Object/ObjectFile.cpp
95

Hmm.. the false is better option anyway.

djtodoro updated this revision to Diff 250782.Mar 17 2020, 8:10 AM

-addressing a comment

aprantl added inline comments.Mar 17 2020, 9:32 AM
llvm/lib/Object/COFFObjectFile.cpp
332

You might want to cross-reference this with MCObjectFileInfo.cpp which seems to hardcode these values, too. It looks like this is what is used for DWARF on COFF

djtodoro updated this revision to Diff 251014.Mar 18 2020, 1:52 AM

-addressing a comment

aprantl accepted this revision.Mar 18 2020, 8:45 AM

You could add a unit test for this, but it would effectively be a reimplementation of the function. I'm fine either way.

This revision is now accepted and ready to land.Mar 18 2020, 8:45 AM
jhenderson accepted this revision.Mar 19 2020, 3:04 AM

LGTM, but I'd like to see at least one other change that makes use of this to be ready before you commit this, as otherwise this is just dead and untested code. I agree a unit test probably doesn't make much sense.

Can this go as is, since the D74205 has a few tests using this? I agree a unit test is not the best solution for this..

Can this go as is, since the D74205 has a few tests using this? I agree a unit test is not the best solution for this..

Right, sorry for the confusion. Now that D74205 is ready to land, this is good to go.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2020, 2:09 AM