diff mbox series

[2/2] libcpp: Avoid remapping filenames within directives

Message ID 20220817121534.1825108-2-richard.purdie@linuxfoundation.org
State New, archived
Headers
Series [1/2] gcc/file-prefix-map: Allow remapping of relative paths |

Commit Message

Richard Purdie Aug. 17, 2022, 12:15 p.m. UTC 
  Code such as:

can interact poorly with file-prefix-map options when cross compiling. In
general you're after to remap filenames for use in target context but the
local paths should be used to find include files at compile time. Ingoring
filename remapping for directives is one way to avoid such failures.

libcpp/ChangeLog:

    * macro.cc (_cpp_builtin_macro_text): Don't remap filenames within directives

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 libcpp/macro.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I wasn't sure if this should apply to all directives or whether this may need
filtering to includes (along the lines of pfile->directive.flags & INCL). That
gets a little more complex as the flag isn't available outside of directives.cc
and would need a helper function.

Comments

Richard Purdie Aug. 17, 2022, 2:19 p.m. UTC | #1 
On Wed, 2022-08-17 at 13:15 +0100, Richard Purdie via Gcc-patches
wrote:
> Code such as:
> 
> can interact poorly with file-prefix-map options when cross compiling. In
> general you're after to remap filenames for use in target context but the
> local paths should be used to find include files at compile time. Ingoring
> filename remapping for directives is one way to avoid such failures.
> 
> libcpp/ChangeLog:
> 
>     * macro.cc (_cpp_builtin_macro_text): Don't remap filenames within directives
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  libcpp/macro.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I wasn't sure if this should apply to all directives or whether this may need
> filtering to includes (along the lines of pfile->directive.flags & INCL). That
> gets a little more complex as the flag isn't available outside of directives.cc
> and would need a helper function.

The above should have said 

Code such as:

#include __FILE__

can interact poorly with...

Cheers,

Richard
Jeff Law Nov. 1, 2022, 7:32 p.m. UTC | #2 
On 8/17/22 06:15, Richard Purdie via Gcc-patches wrote:
> Code such as:
#include __FILE__
>
> can interact poorly with file-prefix-map options when cross compiling. In
> general you're after to remap filenames for use in target context but the
> local paths should be used to find include files at compile time. Ingoring
> filename remapping for directives is one way to avoid such failures.
>
> libcpp/ChangeLog:
>
>      * macro.cc (_cpp_builtin_macro_text): Don't remap filenames within directives

So I went back and reviewed the old PR which introduced this code.  It 
was actually the Yocto project that got this code in to begin with :-)  
There wasn't really any discussion AFAICT about whether or not to remap 
in directives that I saw in the PR.


ISTM that given the change in behavior, we should probably document that 
we don't remap in directives.  Probably doc/invoke.texi.

With suitable documentation, this should be fine.  It seems like it 
ought to be independent of the first patch in this series which adds 
support for remapping relative paths.


jeff
Richard Purdie Nov. 2, 2022, 10:52 a.m. UTC | #3 
On Tue, 2022-11-01 at 13:32 -0600, Jeff Law wrote:
> On 8/17/22 06:15, Richard Purdie via Gcc-patches wrote:
> > Code such as:
> #include __FILE__
> > 
> > can interact poorly with file-prefix-map options when cross compiling. In
> > general you're after to remap filenames for use in target context but the
> > local paths should be used to find include files at compile time. Ingoring
> > filename remapping for directives is one way to avoid such failures.
> > 
> > libcpp/ChangeLog:
> > 
> >      * macro.cc (_cpp_builtin_macro_text): Don't remap filenames within directives
> 
> So I went back and reviewed the old PR which introduced this code.  It 
> was actually the Yocto project that got this code in to begin with :-)

Thanks for the review!

That sounds right, we use it heavily and originally had a few issues in
this area. It now generally works really well, we just found this
corner case :)
  
> There wasn't really any discussion AFAICT about whether or not to remap 
> in directives that I saw in the PR.

I don't think we'd realised there was this corner case. Now we have
found code doing it, I think the behaviour we should have is fairly
clear which is why we're sending the patch.

> ISTM that given the change in behavior, we should probably document that 
> we don't remap in directives.  Probably doc/invoke.texi.
> 
> With suitable documentation, this should be fine.  It seems like it 
> ought to be independent of the first patch in this series which adds 
> support for remapping relative paths.

Thanks for merging 1/2, it was independent, just related as a path
mapping issue we found. I'll keep the coding style in mind in future.

I've sent a new version of this patch which updates doc/invoke.texi,
just adding to file-prefix-map as the others all reference it.

Cheers,

Richard
diff mbox series

Patch

diff --git a/libcpp/macro.cc b/libcpp/macro.cc
index 8ebf360c03c..7d5a0d0fd2e 100644
--- a/libcpp/macro.cc
+++ b/libcpp/macro.cc
@@ -563,7 +563,7 @@  _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node,
 	    if (!name)
 	      abort ();
 	  }
-	if (pfile->cb.remap_filename)
+	if (pfile->cb.remap_filename && !pfile->state.in_directive)
 	  name = pfile->cb.remap_filename (name);
 	len = strlen (name);
 	buf = _cpp_unaligned_alloc (pfile, len * 2 + 3);