On Tue, 16 Jan 2024 12:33:42 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:

>> Andrew Leonard has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains four additional 
>> commits since the last revision:
>> 
>>  - 8323667: Library debug files contain non-reproducible full gcc include 
>> paths
>>    
>>    Signed-off-by: Andrew Leonard <anleo...@redhat.com>
>>  - 8323667: Library debug files contain non-reproducible full gcc include 
>> paths
>>    
>>    Signed-off-by: Andrew Leonard <anleo...@redhat.com>
>>  - Merge branch 'master' of github.com:openjdk/jdk into gcc_debug_map
>>  - 8323667: Library debug files contain non-reproducible full gcc include 
>> paths
>>    
>>    Signed-off-by: Andrew Leonard <anleo...@redhat.com>
>
> make/autoconf/flags-cflags.m4 line 124:
> 
>> 122:     fi
>> 123: 
>> 124:     if test "x$DEBUG_PREFIX_CFLAGS" != x; then
> 
> Should this test not be inside the "if $ALLOW_ABSOLUTE_PATHS_IN_OUTPUT == 
> false" block?
> 
> edit: Ah, I see, this is a consequence of the variables set in that if block.

The logic of this is a bit strange. I think what we really want to express is:

if ! ALLOW_ABSOLUTE_PATHS_IN_OUTPUT and -fdebug-prefix-map is a valid option 
for gcc then
  set -fdebug-prefix-map for both workspace_root and gcc system libraries
endif

Now this connection is kind of lost. Maybe the easiest way to do this is just 
to do the FLAGS_COMPILER_CHECK_ARGUMENTS separately, and reading the result of 
that check as a boolean variable in a separate if statement, since nesting much 
code inside a `IF_TRUE:` block can be hard to read. Or move the setting of the 
workspace_root as well to DEBUG_PREFIX_MAP_GCC_INCLUDE_PATHS, perhaps renaming 
it in the process, and calling it from `IF_TRUE:`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17399#discussion_r1453385973

Reply via email to