Use relative includes to allow source-based dependencies without -I

[georgthegreat]

We store libunwind code inside monorepo and would like to reduce the overall amount of -I flags.

[llvmbot]

@llvm/pr-subscribers-libunwind

Author: Yuriy Chernyshov (georgthegreat)

Changes

We store libunwind code inside monorepo and would like to reduce the overall amount of -I flags.

---

Full diff: https://github.com/llvm/llvm-project/pull/80443.diff

2 Files Affected:

• (modified) libunwind/include/libunwind.h (+1-1)
• (modified) libunwind/include/unwind.h (+3-3)

diff --git a/libunwind/include/libunwind.h b/libunwind/include/libunwind.h
index b2dae8feed9a3..02739b384e9da 100644
--- a/libunwind/include/libunwind.h
+++ b/libunwind/include/libunwind.h
@@ -13,7 +13,7 @@
 #ifndef __LIBUNWIND__
 #define __LIBUNWIND__
 
-#include <__libunwind_config.h>
+#include "__libunwind_config.h"
 
 #include <stdint.h>
 #include <stddef.h>
diff --git a/libunwind/include/unwind.h b/libunwind/include/unwind.h
index b1775d3a3decc..1973c5826d3ca 100644
--- a/libunwind/include/unwind.h
+++ b/libunwind/include/unwind.h
@@ -13,7 +13,7 @@
 #ifndef __UNWIND_H__
 #define __UNWIND_H__
 
-#include <__libunwind_config.h>
+#include "__libunwind_config.h"
 
 #include <stdint.h>
 #include <stddef.h>
@@ -56,9 +56,9 @@ typedef enum {
 typedef struct _Unwind_Context _Unwind_Context;   // opaque
 
 #if defined(_LIBUNWIND_ARM_EHABI)
-#include <unwind_arm_ehabi.h>
+#include "unwind_arm_ehabi.h"
 #else
-#include <unwind_itanium.h>
+#include "unwind_itanium.h"
 #endif
 
 typedef _Unwind_Reason_Code (*_Unwind_Stop_Fn)

[ldionne]

We require that the right paths be setup with -I or -isystem as a general requirement of libc++, libc++abi and libunwind. This change seems brittle to me, especially if there's no test or no official guarantee being added and documented.

Basically, this might fix things for you in the immediate term, but it's really just a band-aid and it will break again in the future. Can you please provide more context about your use case and what official change you'd like us to make (now, but also going forward in the code base). Then, if we agree this is worth pursuing, we should set up some kind of test that allows us to provide that property in a guaranteed way and not regress it.

[georgthegreat]

We have pretty large codebase and we use custom command-graph based build system (consider bazel as a well-known example of such buld system). The system uses module as a basic unit, modules might depend one each other, dependencies might affect dependant modules (but not vice versa).

In order to reduce the total size of the command graph, we would like to reduce the amount of -I flags induced by the most common modules.

libunwind is one of such modules: almost every module transitively depend on it yet very little do include unwind.h directly.

We #include unwind.h using a repository-root-relative-path (i. e. contrib/libs/libunwind/include/unwind.h).
Everything else just works if we apply this patch.

[georgthegreat]

I understand the this will definitely break in the future without proper testing, but libunwind does not experience frequent changes, so this might be a working soluiton even without explicit testing.

[ldionne]

Doing ad-hoc changes like that to provide an apparent guarantee without actually documenting it, testing it and enforcing it throughout is just churning the code base. Unless we actually have a plan to do that properly, I would push back this change. I understand you're just trying to fix your specific problem, but this is not a scalable way to handle changes in a project with so many users.

[georgthegreat]

I am OK with fixing this particular guarantee with a test.

However, I am not familiar with libc++ testset organization and I need to be guided.
Could you, please, provide me with some clues to start wtih?

[arichardson]

It does seem like all other libunwind sources use #include " to find headers that are part of libunwind, so I think doing it for the includes that are installed should also be fine. I don't think installing these headers into separate directories and adding custom -isystem flags is a supported build configuration. It will also ensure that we always pick the matching header rather than one that happens to come first in the include order.

[georgthegreat]

@ldionne, do you find @arichardson reasoning good enough to proceed with merging this PR?

[georgthegreat]

@ldionne, gentle ping

[georgthegreat]

@ldionne, gentle ping

[ldionne]

@ldionne, do you find @arichardson reasoning good enough to proceed with merging this PR?

I'm not convinced. We use #include <header> pretty consistently in libc++ and libc++abi and we should strive for all the runtimes to be consistent. I am especially not convinced that the end goal (of avoiding a -I flag) is worthwhile.