Page MenuHomePhabricator

Declare library dependency introduced by https://reviews.llvm.org/D59634
AbandonedPublic

Authored by serge-sans-paille on Apr 2 2019, 6:28 AM.

Details

Reviewers
labath
jhenderson

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 6:28 AM
labath requested changes to this revision.Apr 2 2019, 6:33 AM

Thanks. I was just looking at bot failure I got and about to submit a fix.

I don't believe this is the right fix though. The correct way to add dependencies should be through LLVMBuild.txt. I am going to submit that soon.

This revision now requires changes to proceed.Apr 2 2019, 6:33 AM
jhenderson requested changes to this revision.Apr 2 2019, 6:35 AM

I can't see how D59482 introduces a dependency on LLVMObject in the main LLVMObjectYAML library. It does add unit tests that rely on the Object library, and includes the necessary reference. Where is the new dependency that this is needed for?

labath added a comment.Apr 2 2019, 6:42 AM

I can't see how D59482 introduces a dependency on LLVMObject in the main LLVMObjectYAML library. It does add unit tests that rely on the Object library, and includes the necessary reference. Where is the new dependency that this is needed for?

I think serge has linked the wrong patch. The culprit is https://reviews.llvm.org/D59634, which does add the dependency because it uses the Object library to construct the ObjectYAML model. This is a new dependency because for all the other binary formats the code to construct the yaml model lives in tool code (obj2yaml). Here, I chose to do that in the ObjectYAML library because I wanted to make that code accessible to unit tests.

@jhenderson do you see any problems with this new dependency (i.e., should I push a patch adding it), or does it need more discussion (i.e., revert the original patch) ? (FWIW, to me it seems natural for ObjectYAML to depend on Object, and I don't think this creates any danger of dependency cycles, as Object should never depend on ObjectYAML.)

I can't see how D59482 introduces a dependency on LLVMObject in the main LLVMObjectYAML library. It does add unit tests that rely on the Object library, and includes the necessary reference. Where is the new dependency that this is needed for?

I think serge has linked the wrong patch. The culprit is https://reviews.llvm.org/D59634, which does add the dependency because it uses the Object library to construct the ObjectYAML model. This is a new dependency because for all the other binary formats the code to construct the yaml model lives in tool code (obj2yaml). Here, I chose to do that in the ObjectYAML library because I wanted to make that code accessible to unit tests.

Ah, that makes more sense, thanks.

@jhenderson do you see any problems with this new dependency (i.e., should I push a patch adding it), or does it need more discussion (i.e., revert the original patch) ? (FWIW, to me it seems natural for ObjectYAML to depend on Object, and I don't think this creates any danger of dependency cycles, as Object should never depend on ObjectYAML.)

My main concern is the number of files needed to build obj2yaml/yaml2obj, which would dramatically increase with that dependency. That being said, most of those files would be needed to be built for the tools that these two executables are used to test, so I don't think it's a genuine issue, and what you say about naturalness does make sense, so please go ahead and fix it.

labath added a comment.Apr 2 2019, 6:56 AM

My main concern is the number of files needed to build obj2yaml/yaml2obj, which would dramatically increase with that dependency. That being said, most of those files would be needed to be built for the tools that these two executables are used to test, so I don't think it's a genuine issue, and what you say about naturalness does make sense, so please go ahead and fix it.

Thanks. It looks like somebody already beat me to it (r357471).

I don't think this will have any affect on the dependency graph of these two tools, as they already have this dependency in their own CMakeLists.txt files. This makes total sense for obj2yaml, as it uses the object library to parse the object files. I'm not entirely sure why this is needed for yaml2obj, but the dependency seems to be coming from the wasm and coff code.

serge-sans-paille retitled this revision from Declare library dependency introduced by https://reviews.llvm.org/D59482 to Declare library dependency introduced by https://reviews.llvm.org/D59634.Apr 2 2019, 7:18 AM
serge-sans-paille abandoned this revision.Apr 2 2019, 7:20 AM

Obsoleted by r357471